Global Styles: Add block-level Text Alignment UI #61717
Conversation
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ phpunit/class-wp-theme-json-test.php |
t-hamano
added
[Type] Enhancement
t-hamano
force-pushed
the
global-styles-text-align
branch
from
34b472d
to
b0e4293
Compare
|
Size Change: -35 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
| // Text alignment is only available for blocks. | ||
| if ( element ) { | ||
| updatedSettings.typography.textAlign = false; | ||
| } |
There was a problem hiding this comment.
This code is necessary to prevent the text alignment UI from being displayed in the Text, Links, Headings, Captions and Buttons typography panels.
t-hamano
force-pushed
the
global-styles-text-align
branch
from
b0e4293
to
1707d2f
Compare
t-hamano
force-pushed
the
global-styles-text-align
branch
from
1707d2f
to
0c1eb05
Compare
| // If `panelId` has a value, it means this panel will be rendered inside | ||
| // the block sidebar. The text alignment UI for individual blocks is rendered | ||
| // in the block toolbar, so disable support if it's not in the global style. | ||
| if ( panelId ) { | ||
| settings.typography.textAlign = false; | ||
| } |
There was a problem hiding this comment.
This was the point that bothered me the most. This prevents duplicate UI in inserted blocks, but there may be a better approach.
There was a problem hiding this comment.
Rather than relying on fudging the settings.typography.textAlign value, could we leverage the fact that this typography panel is extended by the site editor's global styles typography panel?
Perhaps the text alignment control could be rendered only where it is needed that way.
There was a problem hiding this comment.
I tried a different approach with 9c4be9e.
In BlockStyleControls, override settings and disable textAlign support. This component is used to render sidebars for individual blocks, so it does not affect the global styles. This will cause the TypographyPanel to assume it does not support textAlign and will not render the UI.
I can't think of any other good approach at the moment, do you have any ideas?
There was a problem hiding this comment.
I can't think of any other good approach at the moment, do you have any ideas?
Only the suggestion in my last comment. Did you explore adding the global styles only control as a child to the global styles typography panel?
It makes sense to me that if we have a component extending the typography panel for the site editor's global styles panel that something specific to that is added there instead of hacking the settings in an unexpected manner.
There was a problem hiding this comment.
Did you explore adding the global styles only control as a child to the global styles typography panel?
The method I came up with is to pass the context to StylesTypographyPanel in the block screen as shown below.
--- a/packages/edit-site/src/components/global-styles/screen-block.js
+++ b/packages/edit-site/src/components/global-styles/screen-block.js
@@ -260,6 +260,7 @@ function ScreenBlock( { name, variation } ) {
value={ style }
onChange={ setStyle }
settings={ settings }
+ context="global-styles"
/>
) }
{ hasDimensionsPanel && (And depending on this context, the typography panel controls the UI.
diff --git a/packages/block-editor/src/components/global-styles/typography-panel.js b/packages/block-editor/src/components/global-styles/typography-panel.js
index 3106723945..bbd10e97b2 100644
--- a/packages/block-editor/src/components/global-styles/typography-panel.js
+++ b/packages/block-editor/src/components/global-styles/typography-panel.js
@@ -173,7 +173,11 @@ export default function TypographyPanel( {
settings,
panelId,
defaultControls = DEFAULT_CONTROLS,
+ context,
} ) {
+ if ( context === 'global-styles' ) {
+ // Do something...
+ }
const decodeValue = ( rawValue ) =>
getValueFromVariable( { settings }, '', rawValue );Is this your intended approach? Sorry if I misunderstood.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 0c1eb05. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9112888405
|
There was a problem hiding this comment.
Thanks for continuing to iterate on the text alignment support @t-hamano 👍
For the most part, this tests pretty well. I did run into one issue though and left another suggestion in response to your comment.
✅ Root global styles UI does not include text alignment controls
✅ Text alignment controls show for block types under Global Styles
❓ Theme.json root styles for text align apply. Should they?
✅ Theme.json block type styles for text align work
✅ Theme.json block type styles correctly show as default within Global Styles
✅ Block instance text alignment overrides values set in theme.json or Global Styles
I'm missing some of the context and discussion around what should or shouldn't be allowed with text alignment in Global Styles. So take this next comment with a grain of salt.
If we need stricter controls around root global styles and disallowing text alignment at that level, perhaps we could just omit the top level typography.textAlign style during theme.json sanitization.
| // If `panelId` has a value, it means this panel will be rendered inside | ||
| // the block sidebar. The text alignment UI for individual blocks is rendered | ||
| // in the block toolbar, so disable support if it's not in the global style. | ||
| if ( panelId ) { | ||
| settings.typography.textAlign = false; | ||
| } |
There was a problem hiding this comment.
Rather than relying on fudging the settings.typography.textAlign value, could we leverage the fact that this typography panel is extended by the site editor's global styles typography panel?
Perhaps the text alignment control could be rendered only where it is needed that way.
This seems to be happening with other supports as well. For example, in the global styles Text panel, the UI for Text Transform, Column Count, and Text Decoration will not be displayed.
However, if you define the following style in theme.json, the style will be output as shown below. {
"$schema": "../../schemas/json/theme.json",
"version": 3,
"settings": {
"appearanceTools": true,
"layout": {
"contentSize": "840px",
"wideSize": "1100px"
}
},
"styles": {
"typography": {
"textTransform": "uppercase",
"textColumns": "2",
"textDecoration": "underline"
}
}
}
Should this be resolved as a separate issue? |
Yeah, there's probably a bigger discussion to be had around the expectations here. In some cases, I can see theme authors wishing to disable the UI but set their own style that won't be messed with, especially at the block level. At the root level is where I wonder if there are some styles that simply don't make sense and should be prevented completely. |
|
Update: I added a backport changelog. |
|
@aaronrobertshaw I would appreciate your re-review so that this PR can be shipped to WP6.6 🙏
This is the best approach I could come up with for now to achieve this, if there's anything I've missed I'd really appreciate some advice. |
Apologies @t-hamano, I've been stuck getting the section styling feature to a place it can land for WP6.6. I'll try and give it a review this afternoon once that feature is off my plate. |
@t-hamano this approach still feels a bit hacky. My suggestion was to make the global styles typography panel accept children such that the control can be explicitly added only when required. No unexpected overrides to settings would be needed then. I think that is an implementation detail that could perhaps be followed up on post WP6.6. I'll give this a review shortly focused only on the UX/functional side of it. |
|
@aaronrobertshaw Thanks for the advice! The reason I'm adamant about overriding the We add a prop to the TypographyPanel to hide the text alignment UI if certain conditions are met. But Sorry if I didn't explain it well 🙏 |
|
Thanks for the explanation. It really wasn't clear why the approach was taken without it. 👍 Unfortunately, it appears as though we have an issue with the application of the text alignment support. The CSS rules for the Once text alignment styles are added via theme.json or global styles, they can't be overridden on the instance. Screen.Recording.2024-05-31.at.4.05.37.PM.mp4 |
|
In terms of the Global Styles UI change this is looking good. I'm just not sure whether we can push ahead with this without fixing the block instance styles first. I have to go AFK shortly so might not be able to help push this one across the line. |
There was a problem hiding this comment.
@t-hamano I wonder if the text alignment support needs to work more like other block supports. Those supports tend to use presets which involve the !important property to guarantee they get applied over random stylesheet values or global styles.
When I last tested this and block instance styles worked, it was due to global styles at that time using zero-specificity by wrapping selectors in :where(). The approach to zero specificity had to be changed for WP6.6 due to a number of issues such as reset styles bleeding into things. More discussion on that can be found over in #61810.
The only two options I can think of at the moment is to bump specificity or to use different classes to apply the block support e.g. has-block-text-align-* which could avoid some potential BC implications of changing the original classes.
Using new classes means that migrating current static blocks would require a deprecation. I know avoiding that was a reason for using the existing classes.
One important aspect here to consider is that this issue will be present whether or not this PR is merged. It will need to be fixed in Gutenberg and WP6.6 regardless.
I believe this bug fix can be a separate follow-up during the beta period.
For that reason, I'm giving this PR an approval. If you're happy with that @t-hamano and have the bandwidth to follow-up, I'd say go ahead and merge this.
|
Thank you for the feedback!
Yes, I came to this conclusion too after looking into why the problem occurred. I'd like to address that issue in a follow-up, so I'd like to merge this PR. I'd like to try some of the ideas you mentioned. |
Closes #60762
Follow up #59531
What?
This PR adds block-level Text Alignment UI in the Global Styles.
Why?
#59531 added textAlign as block support. However, I think it would be useful to be able to change the default alignment in the GlobalStyles UI.
How?
This UI only appears in the block-level typography panel. In other words, it will not appear on Typography > Text, Links, Headings, Captions and Buttons.
Testing Instructions
Currently, no core blocks exist that have this support. Add support to Media & Text blocks to test this PR.
Details
Enable Emptytheme and define it in theme.json as follows. This enables textAlign support and changes the default textAlign of the Media & Text block.
Note
#61182 will change the default for this support to
true.Details
{ "$schema": "../../schemas/json/theme.json", "version": 3, "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" }, "typography": { "textAlign": true } }, "styles": { "blocks": { "core/media-text": { "typography": { "textAlign": "center" } } } } }Screenshots or screencast
08042c0b201319b15f7f02232a0bdd28.mp4