Enhancing Quote block styling for different alignments #11646
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,47 @@ | ||
| .wp-block-quote { | ||
| margin: 0 inherit; | ||
|
|
||
| &__citation { | ||
| font-size: $default-font-size; | ||
| } // &__citation | ||
|
|
||
| &.is-left { | ||
| border-left: 4px solid $black; | ||
| padding-left: 1em; | ||
| } // &.is-left | ||
|
|
||
| &.is-right { | ||
| border-right: 4px solid $black; | ||
| padding-right: 1em; | ||
| text-align: right; | ||
| } // &.is-right | ||
|
|
||
| &.is-center { | ||
|
Member
There was a problem hiding this comment. For consistency with the other quote styles and for easier overriding in themes, I think it would make sense to simplify the styling of centred quotes. Maybe just a full-width (ie, using a border) top- and bottom- border would work?
Member
Author
There was a problem hiding this comment. I disagree. I do not believe there is anything over complicated about the CSS using pseudo-elements. I believe it affords a nicer, elegant out-of-box look for centered quotes, while also allowing a lot of options for theme authors to overrides
Contributor
There was a problem hiding this comment. I have a small preference for Sarahs design, and would generally agree that simpler is almost always better. Additionally, the double borders makes it look more like the Pullquote block:
Your argument is not wrong, and there's nothing wrong with what you're doing here. In other words, both your points are valid. Separate note on this, this is how Twenty Nineteen looks now:
In that vein it's worth clarifying a thing:
TwentyNineteen opts into those opinionated styles. And those opinionated styles exist exactly so we can do things like these. But this also means we need to move all those visual styles into theme.scss so a theme only gets them if opting in. Right now there are a lot of rules in editor.scss that will be present regardless of theme opt-in.
Member
Author
There was a problem hiding this comment. Yes, all these TwentyNineteen issues were noted in the original PR notes above. Thanks for clarifying though.
Member
There was a problem hiding this comment. There isn't anything complicated, per se, about using pseudo-elements in CSS, but it presents a bit of cognitive dissonance when the three alignments are styled using a different approach. So for example, let's say in my theme I just want to change the colour of the border to be pink.. all I should need to do is apply I'm not opposed to adding extra complexity if it adds value, but from a purely visual perspective, I'm not sure either solution is likely to be better for the majority of users. |
||
| margin: 3em inherit; | ||
| padding-left: 2em; | ||
| padding-right: 2em; | ||
| text-align: center; | ||
|
|
||
| &::before { | ||
| background-color: $black; | ||
| content: ""; | ||
| display: block; | ||
| height: 4px; | ||
| margin-left: calc(50% - 10%); | ||
| margin-bottom: 2em; | ||
| width: 20%; | ||
| } // &::before | ||
|
Contributor
There was a problem hiding this comment. Let's not do these closing comments. They aren't added anywhere else in Gutenberg. |
||
|
|
||
| &::after { | ||
| background-color: $black; | ||
| content: ""; | ||
| display: block; | ||
| height: 4px; | ||
| margin-left: calc(50% - 10%); | ||
| margin-top: 2em; | ||
| width: 20%; | ||
| } // &::after | ||
|
|
||
| } // .is-center | ||
|
|
||
| } // .wp-block-quote | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| * External dependencies | ||
| */ | ||
| import { omit } from 'lodash'; | ||
| import classnames from 'classnames'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
|
|
@@ -33,6 +34,7 @@ const blockAttributes = { | |
| }, | ||
| align: { | ||
| type: 'string', | ||
| default: 'is-left', | ||
|
Member
There was a problem hiding this comment. An unintended consequence of applying a new class as the default like this is that any quotes created prior to this change wouldn't have the class applied—so if you re-open a previously saved left-aligned quote block, it suddenly loses its styling! I think it would be best instead to make the left-aligned styling the default for the block, and override it for the right- and centre-aligned options, even though it does involve writing more CSS to reset things. I'm not sure if we have conventions here either, but I think other blocks tend to follow the "default styling, then overrides" approach. I'd need to do some digging to be sure though!
Member
Author
There was a problem hiding this comment. Hmm, the This was the case before, as newly created Quote blocks had With that said, it could potentially have regressions on prior blocks that were saved as I could likely work up an instance using the
Contributor
There was a problem hiding this comment. This is not a change I'd recommend we do this close to 5.0 release, and it also needs to take RTL languages into account.
Contributor
There was a problem hiding this comment. I agree that there should be no class by default. It seems like the css way to do things in terms of the class being a 'modifier', and modifiers only being applied for a variation of a default. I also think the classes could be more verbose, e.g. 'is-right-aligned', 'is-center-aligned'. |
||
| }, | ||
| }; | ||
|
|
||
|
|
@@ -189,17 +191,23 @@ export const settings = { | |
|
|
||
| edit( { attributes, setAttributes, isSelected, mergeBlocks, onReplace, className } ) { | ||
| const { align, value, citation } = attributes; | ||
|
|
||
| const classes = classnames( | ||
| className, | ||
| align, | ||
| ); | ||
|
|
||
| return ( | ||
| <Fragment> | ||
| <BlockControls> | ||
| <AlignmentToolbar | ||
| value={ align } | ||
| onChange={ ( nextAlign ) => { | ||
| setAttributes( { align: 'is-' + nextAlign } ); | ||
|
Member
There was a problem hiding this comment. If this class is unique to this block, I'd probably recommend namespacing it. Again, not sure offhand what our conventions are here, but I think we tend to namespace pretty enthusiastically.
Member
Author
There was a problem hiding this comment. Yeah, I'm not certain what the conventions are here either. I did peruse some other blocks, and based on things like and I extrapolated:
Member
There was a problem hiding this comment. Thanks for digging into it and providing additional context! It looks like we're sometimes using a data-attribute for alignments, unless I'm misunderstanding it: Would it make sense to use that here as well?
Contributor
There was a problem hiding this comment. It looks like data-attributes are used when the whole block is aligned (i.e floated and not full-width). I'm not sure if it's something we'd consider for text alignment as well. I couldn't find any prior examples where anything other than inlines styles are used for text alignment (paragraph and verse). |
||
| } } | ||
| /> | ||
| </BlockControls> | ||
| <blockquote className={ classes }> | ||
| <RichText | ||
| multiline | ||
| value={ value } | ||
|
|
@@ -240,11 +248,16 @@ export const settings = { | |
| ); | ||
| }, | ||
|
|
||
| save( { attributes, className } ) { | ||
| const { align, value, citation } = attributes; | ||
|
|
||
| const classes = classnames( | ||
| className, | ||
| align, | ||
| ); | ||
|
|
||
| return ( | ||
| <blockquote className={ classes }> | ||
| <RichText.Content multiline value={ value } /> | ||
| { ! RichText.isEmpty( citation ) && <RichText.Content tagName="cite" value={ citation } /> } | ||
| </blockquote> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,19 @@ | ||
| .wp-block-quote { | ||
|
|
||
| &.is-style-large, | ||
| &.is-large { | ||
|
Member
There was a problem hiding this comment. I'd leave I'd apply the border only to the default quote style.
Member
Author
There was a problem hiding this comment. So, this is interesting to me, because currently in Gutenberg Quote block's This screams inconsistency in oversight to me. 😞 Therefore, this is why I went to the extent of trying to update the overall styling of the Quote block component to a little more consistent and scaleable solution. In hopes that it would get merged before Gutenberg launch (WP 5.0). Also, to clarify I changed
Contributor
There was a problem hiding this comment. I deeply appreciate your work here, and while you have several valid points, as we barrel towards the November 27th release, it's critical that we focus pull requests only on solving very specific issues, and that we avoid lumping in unrelated changes. Even though there can be an argument for em changes, it's unrelated to the initial issue, and is only making this review more complex.
Solid observation, worth fixing.
Contributor
There was a problem hiding this comment. I agree that it'd be good to split this out into a separate PR. It really helps in terms of: |
||
|
|
||
| p { | ||
| font-size: 1.33em; | ||
| font-style: italic; | ||
| line-height: 1.6; | ||
| } // p | ||
|
|
||
| cite, | ||
| footer { | ||
| font-size: 1em; | ||
| } // cite, footer | ||
|
|
||
| } // &.is-style-large, &.is-large | ||
|
|
||
| } // .wp-block-quote | ||
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 could introduce unanticipated regressions—I'd leave the margins their original settings unless there's a good reason to adjust them.
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 had to be presumptuous here. I've seen too many inexperienced CSS authors be overly explicit in learning to write CSS with shorthand properties.
Further reading: Harry Roberts "CSS Shorthand Syntax Considered an Anti-Pattern"
The left/right margin seemed overly explicit, unnecessary, and would cause more headaches for theme authors to overwrite with
!importantfurther down the line. I agree that there is potential for "unanticipated regressions" now. However, I project that the possibility of these regressions now within the Gutenberg editor vs the outputted styling and front-end experience would outweigh the potential to allow this overly explicit styling to remain.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.
@sarahmonster thanks for the great feedback. I truly appreciate it.
I tried to provide further reasoning behind my decisions. I would hope and encourage that we get another party's insight. I'm still new to the Gutenberg codebase and I don't want to miss anything in the larger scheme of things.
I hope that my reasoning is not perceived as a "no". I'm mostly just curious what more people think as I'm new to the project and tried to apply my most sound reasoning and experience, while also perusing existing Issues, Git Blame, and general history to get a sense of best practices and approaches.
Hopefully, somebody else will drop some feedback and I can get all this updated in another pass, sooner than later. Thanks again!
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 would agree with Sarah here that this is a little sensitive to introduce at this point, which is right before the 5.0 release. Which means we have to be extra careful not to cause regressions. In this case it's important to test this both with themes that do support wide alignments, and with themes that do not. There's also mobile, tablet, desktop breakpoints to consider, and even though these are editor styles, the frontend should ideally match.
I understand your arguments and reasons for changing this — and I think this would make a good separate pull request that looks at these editor specific margin rules for all the blocks, together. But changing this for just the quote block in isolation is just going to cause inconsistency, even if this becomes the only block that's "right".
In other words, unless this rule is strictly necessary for the purpose of this PR, I would not bundle it into this 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.
@jasmussen @sarahmonster
Thanks for the thorough and insightful review!
One thing still remains unclear. There are several mentions of “causing regressions”. Perhaps I’m missing some oversight on the Gutenberg project’s milestones and project goals. Since it has not been “officially” released yet then why are we concerned with regressions?
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.
You mentioned yourself wanting to get these improvements in the 5.0 release. That release is due November 27th, which is two weeks from now. Which means any change we make between now and then has to be solid and stable, and, well, not cause regressions.
A regression could be:
This is not to say any of those regressions can't be caught and addressed in this PR. It's just that if this PR has a goal of getting merged before the 5.0 release, then it will be held to a higher standard (and should probably be reduced in scope) than if, for example, it goes into the 5.1 release, at which point it's totally fair to expand the scope.
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.
@jasmussen thanks for the clarification. Makes absolute sense.