Fix: Enable Text Align UI to be controlled correctly with theme.json #61182
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
[Type] Bug
3 tasks
|
Size Change: -7 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
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
force-pushed
the
fix/text-align-inheritance
branch
from
3e2052d to
f1288ae
Compare
There was a problem hiding this comment.
Thanks for catching this one and working on the fix @t-hamano 👍
I think we might be re-inventing the wheel a bit with useIsTextAlignDisabled.
When all the block support panels were refactored so the post and site editors share the same component, the approach to checking support and whether the UI was disabled, was also merged.
My understanding is that the theme.json settings are retrieved and then the block specific supports are merged over those.
Take a look at:
Also, I think we need to update the title of this PR to reflect what it's actually about i.e. disabling UI controls rather than the actual inheritance of values etc.
| @@ -70,11 +71,17 @@ function BlockEditTextAlignmentToolbarControlsPure( { | |||
| name: blockName, | |||
| setAttributes, | |||
| } ) { | |||
| const isDisabled = useIsTextAlignDisabled( blockName ); | |||
There was a problem hiding this comment.
I think we might be able to use useBlockSettings here instead of creating a new hook.
t-hamano
changed the title
|
@aaronrobertshaw Thanks for the review!
Admittedly, using this hook is simpler 👍 |
|
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❔ lib/theme.json |
aaronrobertshaw
approved these changes
May 20, 2024
There was a problem hiding this comment.
Thanks for applying the suggested tweaks @t-hamano. Looks good 👍
This still tests well for me:
✅ Classic Theme: UI is visible for both blocks
✅ Block Theme (default): UI is visible for both blocks
✅ Block Theme (disabled): UI is NOT visible for both blocks
✅ Block Theme (disabled by block type): Disabling for individual blocks works as expected
Nit: While reading through the code for the text align hook to refresh my memory, it stuck out that the getValidTextAlignments function is a bit long-winded and could be simplified.
Maybe we could tweak that with something like this?
diff --git a/packages/block-editor/src/hooks/text-align.js b/packages/block-editor/src/hooks/text-align.js
index d1edadfad5..1a0cfe2ecc 100644
--- a/packages/block-editor/src/hooks/text-align.js
+++ b/packages/block-editor/src/hooks/text-align.js
@@ -43,6 +43,7 @@ const TEXT_ALIGNMENT_OPTIONS = [
];
const VALID_TEXT_ALIGNMENTS = [ 'left', 'center', 'right' ];
+const NO_TEXT_ALIGNMENTS = [];
/**
* Returns the valid text alignments.
@@ -54,19 +55,12 @@ const VALID_TEXT_ALIGNMENTS = [ 'left', 'center', 'right' ];
* @return {string[]} Valid text alignments.
*/
export function getValidTextAlignments( blockTextAlign ) {
- let validTextAlignments;
if ( Array.isArray( blockTextAlign ) ) {
- validTextAlignments = VALID_TEXT_ALIGNMENTS.filter( ( textAlign ) =>
+ return VALID_TEXT_ALIGNMENTS.filter( ( textAlign ) =>
blockTextAlign.includes( textAlign )
);
- } else if ( blockTextAlign === true ) {
- // `true` includes all alignments...
- validTextAlignments = [ ...VALID_TEXT_ALIGNMENTS ];
- } else {
- validTextAlignments = [];
}
-
- return validTextAlignments;
+ return blockTextAlign === true ? VALID_TEXT_ALIGNMENTS : NO_TEXT_ALIGNMENTS;
}
function BlockEditTextAlignmentToolbarControlsPure( {
|
@aaronrobertshaw Thanks for the review!
This is certainly easier to understand. The unit test for the function passed locally, so the logic shouldn't have changed at all. |
This was referenced May 21, 2024
|
Ready to backport to the core. |
pento
pushed a commit
to WordPress/wordpress-develop
that referenced
this pull request
Jun 4, 2024
See WordPress/gutenberg#59531. See WordPress/gutenberg#61182. See WordPress/gutenberg#61717. See #6590. Fixes #61256. Props wildworks, ellatrix. git-svn-id: https://develop.svn.wordpress.org/trunk@58314 602fd350-edb4-49c9-b593-d223f7449a82
github-actions bot
pushed a commit
to gilzow/wordpress-performance
that referenced
this pull request
Jun 4, 2024
See WordPress/gutenberg#59531. See WordPress/gutenberg#61182. See WordPress/gutenberg#61717. See WordPress/wordpress-develop#6590. Fixes #61256. Props wildworks, ellatrix. Built from https://develop.svn.wordpress.org/trunk@58314 git-svn-id: https://core.svn.wordpress.org/trunk@57771 1a063a9b-81f0-0310-95a4-ce76da25c4cd
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Jun 4, 2024
See WordPress/gutenberg#59531. See WordPress/gutenberg#61182. See WordPress/gutenberg#61717. See WordPress/wordpress-develop#6590. Fixes #61256. Props wildworks, ellatrix. Built from https://develop.svn.wordpress.org/trunk@58314 git-svn-id: http://core.svn.wordpress.org/trunk@57771 1a063a9b-81f0-0310-95a4-ce76da25c4cd
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…ordPress#61182) * Fix: Text Align block support inheritance * Use `useBlockSettings` instead of creating a new hook * Simplify `getValidTextAlignments` function Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
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.
Follow-up #59531
What?
This PR allows
textAlignblock support to be controlled via theme.json.Why?
We added the new
textAlignblock support in #59531. However, since enabling this support depended only on block support, it turned out that it could not be disabled via theme.json. In other words, we cannot disable this block support even if you write code like the following:block.json
{ "apiVersion": 3, "name": "core/test", "title": "Test", "supports": { "typography": { "textAlign": true } } }theme.json
{ "version": 3, "settings": { "typography": { "textAlign": false } } }How?
Checks whether theme.json and block.json are supported, and displays the block toolbar UI if both support it. This logic is implemented as a
useIsTextAlignDisabledhook and is similar to the logic in useIsFontSizeDisabled, useIsLineHeightDisabled, etc.Testing Instructions
Temporarily add this support to the Media & Text block and the Group block:
Diff
Default (Classic Theme)
Default (Theme with theme.json)
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 3, "settings": { "layout": { "contentSize": "840px" } } }Disable at root level of theme.json
Activate Emptytheme.
Define theme.json as below.
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 3, "settings": { "layout": { "contentSize": "840px" }, "typography": { "textAlign": false } } }The UI should NOT be visible in both blocks.
Disable at block level of theme.json
Activate Emptytheme.
Define theme.json as below.
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 3, "settings": { "layout": { "contentSize": "840px" }, "blocks": { "core/media-text": { "typography": { "textAlign": false } } } } }Only the Media and Text block should not display this UI.