-
Notifications
You must be signed in to change notification settings - Fork 3
[Varya] Refine quote and pullquote block styles #32
Conversation
--pullquote--color-border: var(--global--color-foreground); | ||
--pullquote--font-size: var(--heading--font-size-h2); | ||
--pullquote--font-style: italic; | ||
--pullquote--line-height: var(--global--line-height-heading); |
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.
I added two variables here, not entirely sure if they're necessary.
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.
Hmmm, I like the line-height
variable. The italic one might be useful too, but just need to be careful that it doesn’t interfere with any inline-italic rules set by the users (which may be a problem already).
@@ -35,7 +35,7 @@ | |||
/* Line Height */ | |||
--global--line-height-base: 1; | |||
--global--line-height-body: 1.7; | |||
--global--line-height-heading: 1.125; | |||
--global--line-height-heading: 1.3; |
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.
I adjusted the value of this global variable to match the H1 - H3 line height from the comps.
The line heights for the headings overall need to be revisited in a follow-up PR.
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.
Coool. Makes sense 👍
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.
This is a good start! A few changes though:
Right-aligned block quotes lose the border treatment:
In the editor, most of these do not match their front-end appearance. We should always make sure to sync these up whenever we make changes to one or the other:
Also, the spacing in all these cases is a little more than I think we need. The left padding here is 2x the global spacing base. Let's reduce that to 1x:
And for this pullquote variation, the all-around padding is currently 3x the spacing base. Let's reduce that to 2x.
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.
Mostly looks good just have one question/comment before decide on merging this one :-)
varya/assets/css/style-editor.css
Outdated
line-height: var(--global--line-height-h3); | ||
font-size: var(--quote--font-size-large); | ||
font-style: normal; | ||
line-height: 1.6; |
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.
Does line-height need to be connected to a variable here?
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 should be.
I connected it to --global--line-height-body
, which is 1.7 even though the comps suggest 1.6 for this specific element.
We can revisit this after #37.
@kjellr I may have misinterpreted this feedback. Are the right-aligned block quotes supposed to retain a right border? |
Yep! Sorry that wasn't clear. They should retain the single-pixel red border, just have it swapped over to the right side. |
Cool, I restored the right aligned border. This is ready for another review. |
Looks good! I noticed a very minor bug: font sizes for the citations should be |
This PR adjusts the quote and pullquote block styles to match the design comps.
Before
After