-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Add font patterns PoC #76
Conversation
@danalvrz @sneridagh I really like this pattern thing and they way we approach styling with it. |
@steffenri @danalvrz FYI: the naming "one, two", "first, second" was just a placeholder since we did not want to concern ourselves with finding proper names. We still have to come up with proper names that make more sense. |
@sneridagh I made some changes to test the mapping, and combined approaches (semantic/descriptive). We can edit it down, or make any adjustments needed. |
@danalvrz LGTM! I would want to know what's your opinion and the DX of this approach. Let's talk later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I think this looks great and the use of mixins seems really nice for when we want to reuse the same styles with multiple selectors.
I made a few comments/questions but they're all pretty minor and non-blocking.
src/theme/_typo-custom.scss
Outdated
@@ -15,6 +15,10 @@ div { | |||
@include body-text(); | |||
} | |||
|
|||
strong > * { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to do it for children of strong
rather than strong
itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When in Edit mode, the markup changes from just a strong
/em
to nested span
elements, but I changed it to a less general approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danalvrz I see, it's needed then because we have font styles assigned to the generic span
selector. My gut feeling is that we should remove font styles from div
and span
so that they inherit from their container, but let's see what @sneridagh thinks.
line-height: 20px; | ||
color: $black; | ||
padding: 8px 20px; | ||
@include body-text-bold(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth creating a mixin for all the button styles, since we also have buttons that are included in other blocks, etc.
@@ -12,33 +12,37 @@ span, | |||
div { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to take div out of this list; it's a bit surprising for divs to have their own font styles assigned rather than inheriting from their container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sure, we can't style divs alone, as a rule of thumb, anywhere, unless there's no other way to do it.
No description provided.