-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Heading: Migrate to text-align block support #74383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
|
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. |
|
Size Change: +39 B (0%) Total Size: 3.08 MB
ℹ️ View Unchanged
|
test/integration/fixtures/blocks/core__heading_align-textalign__deprecated-1.html
Outdated
Show resolved
Hide resolved
| content, | ||
| anchor, | ||
| textAlign, | ||
| ...( textAlign && { |
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.
Will it still work if we delete ...( textAlign?
content,
anchor,
style: {
typography: {
textAlign,
},
},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.
Without this code, transforms didn't work the same as before.
The test "should preserve the text align block support" failed.
gutenberg/test/e2e/specs/editor/blocks/heading.spec.js
Lines 419 to 435 in 47d7e07
| test( 'should preserve the text align block support', async ( { | |
| editor, | |
| } ) => { | |
| await editor.insertBlock( { | |
| name: 'core/paragraph', | |
| attributes: { | |
| style: { typography: { textAlign: 'right' } }, | |
| content: 'initial content', | |
| }, | |
| } ); | |
| await editor.transformBlockTo( 'core/heading' ); | |
| const headingBlock = ( await editor.getBlocks() )[ 0 ]; | |
| expect( headingBlock.name ).toBe( 'core/heading' ); | |
| expect( | |
| headingBlock.attributes.style.typography.textAlign | |
| ).toBe( 'right' ); | |
| } ); |
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 can't reproduce the problem in my environment.
diff --git a/packages/block-library/src/heading/transforms.js b/packages/block-library/src/heading/transforms.js
index 3554f52ff2..d616051bd7 100644
--- a/packages/block-library/src/heading/transforms.js
+++ b/packages/block-library/src/heading/transforms.js
@@ -29,13 +29,11 @@ const transforms = {
),
content,
anchor,
- ...( textAlign && {
- style: {
- typography: {
- textAlign,
- },
+ style: {
+ typography: {
+ textAlign,
},
- } ),
+ },
} );
} ),
},
@@ -116,13 +114,11 @@ const transforms = {
} )
),
content,
- ...( textAlign && {
- style: {
- typography: {
- textAlign,
- },
+ style: {
+ typography: {
+ textAlign,
},
- } ),
+ },
} );
} ),
},After applying the above changes,
npm run test:e2e -- test/e2e/specs/editor/blocks/heading.spec.js --grep "should preserve the text align block support"
I ran the e2e tests and they seem to pass.
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 change seems to have caused other tests to fail, so it might be better to revert to the previous implementation. Sorry for the confusion 🙏
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
|
@t-hamano |
| <!-- wp:heading {"style":{"typography":{"textAlign":"right"}},"textColor":"primary"} --> | ||
| <h2 class="wp-block-heading has-text-align-right has-primary-color has-text-color">Text 1</h2> | ||
| <!-- /wp:heading --> | ||
|
|
||
| <!-- wp:heading {"style":{"typography":{"textAlign":"center"}},"textColor":"primary"} --> | ||
| <h2 class="wp-block-heading has-text-align-center has-primary-color has-text-color">Text 2</h2> | ||
| <!-- /wp:heading --> | ||
|
|
||
| <!-- wp:heading {"style":{"typography":{"textAlign":"left"}},"textColor":"primary"} --> | ||
| <h2 class="wp-block-heading has-text-align-left has-primary-color has-text-color">Text</h2> | ||
| <!-- /wp:heading --> |
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.
| <!-- wp:heading {"style":{"typography":{"textAlign":"right"}},"textColor":"primary"} --> | |
| <h2 class="wp-block-heading has-text-align-right has-primary-color has-text-color">Text 1</h2> | |
| <!-- /wp:heading --> | |
| <!-- wp:heading {"style":{"typography":{"textAlign":"center"}},"textColor":"primary"} --> | |
| <h2 class="wp-block-heading has-text-align-center has-primary-color has-text-color">Text 2</h2> | |
| <!-- /wp:heading --> | |
| <!-- wp:heading {"style":{"typography":{"textAlign":"left"}},"textColor":"primary"} --> | |
| <h2 class="wp-block-heading has-text-align-left has-primary-color has-text-color">Text</h2> | |
| <!-- /wp:heading --> | |
| <!-- wp:heading {"textAlign":"left"} --> | |
| <h2 class="wp-block-heading has-text-align-left">Heading</h2> | |
| <!-- /wp:heading --> | |
| <!-- wp:heading {"textAlign":"center"} --> | |
| <h2 class="wp-block-heading has-text-align-left">Heading</h2> | |
| <!-- /wp:heading --> | |
| <!-- wp:heading {"textAlign":"right"} --> | |
| <h2 class="wp-block-heading has-text-align-left">Heading</h2> | |
| <!-- /wp:heading --> |
- This fixture tests the migration of the textAlign attribute to textAlign block support, so markup must be tested with the old
textAlignattribute. - The
textColoris not relevant to what we want to test in this PR.
What?
Part of #60763
Migrates the
Headingblock to use the textAlign block support instead of a customtextAlignattribute. As a consequence it also enables global styles support fortextAlignfor theHeading.Why?
The
Headingblock currently implements its own text alignment logic with a custom align attribute, duplicating code that should be handled by the centralizedHeadingblock support. This migration reduces code duplication and consolidates alignment handling across blocks.How?
Replaces the custom logic with the block supports, adds deprecation and fixes transforms.
Testing Instructions
Headingblock and test text alignment (left, center, right)Headingblocks that have alignment set