Try: Add new textAlign block support
#59531
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Feature] Block API
|
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. |
t-hamano
requested review from
youknowriad,
andrewserong,
a team,
ockham,
jorgefilipecosta,
draganescu,
ntsekouras,
ellatrix and
aaronrobertshaw
and removed request for
ajitbohra,
tellthemachines,
youknowriad,
andrewserong,
a team,
ockham and
jorgefilipecosta
|
Size Change: +434 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
…`addAttribute` filter
|
@aaronrobertshaw Thank you for your detailed research and advice! 🙇 I've made the following updates based on your feedback.
The next two files you mentioned have not been updated yet. I've tracked how these two files are used, and since they both seem to be related to global styles, I figured we might not need to make any changes at this time 🤔
Do we need to update these two files as well? |
|
Thanks for the latest updates @t-hamano 👍
I'm a little short on bandwidth to dig in here over the next couple of days but I suspect the constants file would need updating if textAlign can be set via theme.json. I'll give this a proper look and review around the middle of the week, if that's okay? |
Of course, thank you! I'll look into it some more too. |
|
Appreciate your patience @t-hamano 🙇 This is getting close but there's one issue that needs ironing out first. I'd also still like to get a sense of what the plan is regarding supporting this in global styles. If it can be styled on an individual block, most theme authors expect it to be able to be styled by theme.json and therefore also global styles. Have you received any specific design feedback around this? ✅ Tested with the supplied test block as well as adopting it for a few simple core blocks like Media & Text, and Group.
This is the use case that prompted my earlier suggestions to update the Global Styles is in large part just the processed theme.json within the site editor. Without the To replicate the issue, style the Media & Text block in theme.json setting a text align value. Note that when you load the post editor or frontend the block is styled correctly because the stylesheet is created by the PHP class. In the site editor however, the styles are generated by the JS hook, so you won't get the same styles applied to the block there. Now add something like the following to the You should then see the correct styles when you refresh the site editor. |
|
@aaronrobertshaw Thank you for your additional feedback 🙏
I was able to reproduce this issue using the theme.json below: {
"$schema": "https://raw.githubusercontent.com/WordPress/gutenberg/add/text-align-block-support/schemas/json/theme.json",
"version": 2,
"styles": {
"blocks": {
"core/media-text": {
"typography": {
"textAlign": "center"
}
}
}
}
}After updating
Regarding the block level, there is currently no design feedback, but I hope it will be displayed as a list of buttons, similar to Decoration and Letter Case.
Also, this PR initially attempted to support Instead, it is proposed to support Do you think adding textAlign UI to the Global Styles should be considered in this PR? |
aaronrobertshaw
approved these changes
Apr 15, 2024
There was a problem hiding this comment.
I think this PR is ready to go @t-hamano, nice work 🚢
After the latest update, theme.json based styling for text align is working correctly in the site editor for me. A quick smoke test again on the other aspects of the block support didn't show any issues.
Do you think adding textAlign UI to the Global Styles should be considered in this PR?
I don't think the lack of Global Styles support should be considered a blocker for this PR.
It can be a follow-up but probably should be added before text align support is offered via WP6.6. There is some time yet before the beta so it should be fine.
|
Thank you everyone for your reviews! I think this PR is ready to ship, so I would like to merge it.
I see, I would like to continue working on the following tasks (please let me know if I have misunderstood anything).
|
This was referenced Apr 16, 2024
|
Sounds like a plan @t-hamano 👍 My vote would be to lean into sorting out the Global Styles UI first, then update the core blocks to adopt the support after that. |
This was referenced Apr 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #38427
Related to #8450, #48202
What?
This PR attempts to add a new
typography.textAlign(left/center/right/justify) block support. I only have a basic implementation at the moment, but I'd love to hear your thoughts on whether this approach makes sense!Why?
The current text-align setting is implemented ad hoc in each block. In other words, the code looks like this:
Code like this exists in over 20 core blocks, so I think it makes sense to unify the block support.
I also think there are two benefits for developers:
How?
The implementation itself is very similar to
alignsupport, with the following characteristics:boolean | arraycan be defined as a value.true, allleft/center/right/~~justify~~options can be controlled.array, only the options that match the array elements can be controlled.default, the toolbar control will not be available.Additionally, if we replace this support in the core blocks in the future, the markup saved will remain the same, so there should be no need to add a deprecation.Update: For all blocks, if text-align changes, the class namehas-text-align-{align}is added. This PR, on the other hand, is output as an inline style. Therefore, we will need to add deprecations in every block.However, I think we should note the following points:
The only exception is the Paragraph block, which is defined as an attribute calledaligninstead oftextAlign. Therefore, it will be necessary to add deprecation only to this block, and adjust the alignment inheritance when transforming from/to the heading block.Testing Instructions
Block Support
This PR purely adds block support only and has no impact on core blocks. Use the code below to register a block that supports
textAlignvia the browser console and check the operation of the block toolbar.Code
Additionally, to ensure that core blocks can migrate to this support in the future, we will temporarily make the following changes to ensure that they work with a variety of block types.
Static Block (Media & Text Block)
It should work as expected, both on the editor and on the frontend.
Dynamic Block (Archives Block)
It should work as expected, both on the editor and on the frontend.
Dynamic Block with __experimentalSkipSerialization (Search Block)
This block skips serializing typography support and gives each label, input field, and button a typography-related class name. On the editor side, the
text-align-{textAlign}class should not be output to the block wrapper div, but instead the class should be output for each label, input field, and button.Pasting Styles
text-alignsetting toright.text-alignsetting (right) should be applied.Screenshots or screencast
The video below shows that a block with temporary
textAlignsupport is registered via the browser console andtextAlignsupport works correctly.8c71ff29436ffdf74d9edb4556bde32f.mp4
ToDo
If it makes sense to move forward with this PR, I would like to work on the following tasks.