Enhancing Quote block styling for different alignments #11646
Conversation
There was a problem hiding this comment.
Hi @colorful-tones! Thanks for the pull request. I've gone through and left some comments, but overall it looks good.
Whilst I think this is an overall improvement, it may not be something we can merge just yet due to our UI freeze. I'm going to loop in @jasmussen as well to take a look at some of the CSS issues, since he'll likely have a much better concept of any potential conflicts there.
| text-align: right; | ||
| } // &.is-right | ||
|
|
||
| &.is-center { |
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?
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 border-color, border-width, width and more.
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:
- editor.scss is always loaded by the editor
- style.scss is always loaded by the editor, and is always loaded by the frontend
- theme.scss is only loaded in editor and frontend if the theme opts into Gutenberg "opinionated styles"
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.
There was a problem hiding this comment.
Yes, all these TwentyNineteen issues were noted in the original PR notes above. Thanks for clarifying though.
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 border-color: pink to the .is-large-quote element in my theme, and I'm done.
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.
| @@ -1,7 +1,47 @@ | |||
| .wp-block-quote { | |||
| margin: 0; | |||
| margin: 0 inherit; | |||
There was a problem hiding this comment.
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.
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 !important further 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.
@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.
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.
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.
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?
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:
- A theme that wants to be Gutenberg aware when 5.0 launches, and has perhaps optimized towards the current implementation of the Quote block, such as TwentyNineteen. (Though we'd probably be able to fix up that specific theme beforehand, but the point is still valid for non twenty themes).
- Regarding the alignments, any existing post authored and saved before this change, when merged, might present an "invalid content" box, or show up as being authored in the Classic block, unless we create a deprecation handler. Creating a deprecation handler is possible, but it's not easy, and it's a relatively big thing to do which means it needs to be thoroughly reviewed and tested, which is hard given there are a lot of other open isssues in the milestones we need to fix for the 5.0 release.
- If a left/right change is made, it could cause a regression for RTL languages where a different default might be wanted.
- If a change to margins is made, it could cause display issues on mobile, or in wide/full-wide configurations. For example, if you put a quote inside a full-wide columns block
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.
@jasmussen thanks for the clarification. Makes absolute sense.
| @@ -33,6 +34,7 @@ const blockAttributes = { | |||
| }, | |||
| align: { | |||
| type: 'string', | |||
| default: 'is-left', | |||
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!
There was a problem hiding this comment.
Hmm, the default: 'is-left' is saying that newly created Quote blocks should automatically have the .is-left class assigned, therefore left-aligned styling applied.
This was the case before, as newly created Quote blocks had style="text-align: left;" automatically applied. So, nothing changes really. Should an editor re-open a previously saved left-aligned quote block it would not suddenly lose its styling. Instead, style="text-align: left;" would be removed and .is-left would get substituted.
With that said, it could potentially have regressions on prior blocks that were saved as style="text-align: right;" or style="text-align: center;", because it would replace those with .is-left automatically.
I could likely work up an instance using the deprecated block API to allow for graceful transition.
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.
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'.
| return ( | ||
| <Fragment> | ||
| <BlockControls> | ||
| <AlignmentToolbar | ||
| value={ align } | ||
| onChange={ ( nextAlign ) => { | ||
| setAttributes( { align: nextAlign } ); | ||
| setAttributes( { align: 'is-' + nextAlign } ); |
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.
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 .is-large, .is-style-large in current Quote block: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/quote/theme.scss
and .is-style-solid-color in current Pullquote block: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/pullquote/editor.scss#L30
I extrapolated: .is-left, .is-center and .is-right. I think these could potentially be reused (and likely should be) within other blocks as well. In this particular instance I don't get a sense that namespacing is typically used from what I've seen of Gutenberg codebase.
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:
https://github.com/WordPress/gutenberg/blob/085a6058eb6cb2c09470dfb1b5fab47b826c48b1/packages/block-library/src/table/editor.scss
https://github.com/WordPress/gutenberg/blob/3a210ab5c127c2a9b9791827515cae3a59452a20/packages/block-library/src/pullquote/editor.scss
(lots more too!)
Would it make sense to use that here as well?
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).
| &.is-style-large, | ||
| &.is-large { | ||
| margin: 0 0 16px; |
There was a problem hiding this comment.
I'd leave .is-style-large and .is-large alone here, and avoid changing the styling. Otherwise, the two quote styles are too similar.
I'd apply the border only to the default quote style.
There was a problem hiding this comment.
So, this is interesting to me, because currently in Gutenberg Quote block's theme.scss we're assigning a margin-bottom of 20px on the overall .wp-block-quote wrapper (see line #2). However, when the Quote block (again, current Gutenberg treatment) gets assigned as .is-large by the user, than we're decreasing the margin-bottom from 20px to 16px here: https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/quote/style.scss#L2
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 px units to em, because using px is making things far too static, and has potential a11y concerns for users on both the back end and the front end.
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.
So, this is interesting to me, because currently in Gutenberg Quote block's theme.scss we're assigning a margin-bottom of 20px on the overall .wp-block-quote wrapper (see line #2). However, when the Quote block (again, current Gutenberg treatment) gets assigned as .is-large by the user, than we're decreasing the margin-bottom from 20px to 16px here:
Solid observation, worth fixing.
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:
a) helping PRs get shipped faster (smaller changes are easier to review)
b) acting as a form of documentation for a future developer who wants to understand the motivation behind a change.
| @@ -1,5 +1,5 @@ | |||
| .wp-block-quote { | |||
| margin: 20px 0; | |||
| margin: 1em inherit; | |||
There was a problem hiding this comment.
Same comment re: changing margins as above—could be fragile & result in unexpected changes.
There was a problem hiding this comment.
Please see previous reasoning:
|
|
||
| .wp-block-quote:not(.is-large):not(.is-style-large) { |
There was a problem hiding this comment.
Let's only apply the borders to the default quote style.
| padding-right: 2em; | ||
| text-align: center; | ||
|
|
||
| &::before { |
There was a problem hiding this comment.
Same comment as above re: the centre-aligned quotes!
| margin-left: calc(50% - 10%); | ||
| margin-bottom: 2em; | ||
| width: 20%; | ||
| } // &::before |
There was a problem hiding this comment.
Let's not do these closing comments. They aren't added anywhere else in Gutenberg.
There was a problem hiding this comment.
Thank you so much for this PR! I really dig the style change Sarah proposed, and thank you for taking a stab at the implementation.
I left a few minor comments around the design implementation. But overall the changes that need to happen for this PR to ship, notably if we mean to ship it soon, are all related to making the entire PR smaller and more focused. Although you have many good arguments, to me those are all separate PRs that look at Gutenberg overall instead of the quote block in isolation. As it is the changes added here are likely to require a deprecation handler, and require testing for RTL, wide/fullwide, mobile, and theme editor styles.
|
I expanded the review range to get you more help in shipping a version of this! |
There was a problem hiding this comment.
Hi @colorful-tones - this looks really good visually, thanks for working on it.
In my testing I spotted an issue. I opened up a post with some existing quotes that had text alignment. I ended up with some block validation issues and the existing content wasn't aligned as expected. Looks like a deprecation for the block is required. Once that's done it'd be good to test whether it resolves all those issues or there's more that needs to be done in terms of compatibility.
Here's what my post looked like originally:
Here's how the same post looked when I switched to this branch:
Newly added blocks worked well though. 👍
| return ( | ||
| <Fragment> | ||
| <BlockControls> | ||
| <AlignmentToolbar | ||
| value={ align } | ||
| onChange={ ( nextAlign ) => { | ||
| setAttributes( { align: nextAlign } ); | ||
| setAttributes( { align: 'is-' + nextAlign } ); |
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).
| &.is-style-large, | ||
| &.is-large { | ||
| margin: 0 0 16px; |
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:
a) helping PRs get shipped faster (smaller changes are easier to review)
b) acting as a form of documentation for a future developer who wants to understand the motivation behind a change.
| @@ -33,6 +34,7 @@ const blockAttributes = { | |||
| }, | |||
| align: { | |||
| type: 'string', | |||
| default: 'is-left', | |||
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'.
|
I'm closing this out in order to re-focus the overall Pull Request for this. Thanks for all the excellent feedback. |
Fixes #11609 and addresses #10970
Description
These changes affect the Quote block.
.is-left(default),.is-center, oris-right) for editor and frontend output.blockquotebased onis-left,is-center, oris-rightm while also accounting for the existing Style option of either: "Regular" or "Large" quotes.How has this been tested?
Changes were tested in local environment with Gutenberg post with all variations of Quote block added. Changes were tested against the TwentySixteen theme and TwentyNineteen theme (currently being developed).
Note: in TwentyNineteen there are already some sane and simple stylistic overrides for Gutenberg Quotes. This PR's changes has a negative stylistic impact on the current styles in TwentyNineteen. However, I would be happy to open and submit a PR to address this minor updates to align with this set of changes.
Tested in Chrome: 70.0.3538.77 on macOS 10.14
Screenshots
Types of changes
Previously the Quote block had the ability to use the BlockControls to align: left, center or right. However, the alignment was being set on the
<blockquote>element withstyle="text-align: right;". I removed this assignment and replaced with className assignment.style="text-align: right;"style="text-align: center;"Checklist:
(via @10up)