Quote: Rename the align attribute to textAlign #42960
Conversation
|
Size Change: +148 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
carolinan
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Can I go ahead and add the block support for alignment in this PR? I can do a second one line PR, but the change relies on this PR to work. |
carolinan
added
[Block] Quote
|
@carolinan It makes sense that we add the block support here to see the full picture. My understanding is that the available values for (wide) align are: left, center, right, wide, full. And the available values for (text) align are: left, center, right. Given the two sets have common values but will produce different serialization, if we have an existing quote with It sounds like if we want to add support for wide align to this block we may need to do it without the block support? cc @ramonjd @andrewserong for layout thoughts. |
|
I added the alignments so that we can test them. |
|
Thanks for the ping!
Oh, great question. Just digging around in other blocks, and I see that the Heading block only opts in to the It was a couple of years ago, but for the Quote block, it sounds like we might be able to borrow a similar technique as used by @ntsekouras in #26492 to fix up the collision in the heading block? This is all with the assumption that the overall objective is to add |
|
Yes the objective is both consistency across blocks and to enable the wide full / left center right alignments. |
|
I might be missing / misunderstanding something important so I'm happy hand this over to anyone :) |
There was a problem hiding this comment.
and to enable the wide full / left center right alignments.
Ah, I see! Unfortunately, due to older block markup using align for storing the text alignment value, I don't think it will be possible to safely opt in to left, center, right in a backwards compatible way, due to the same issues as occurred in the Heading block. I'd recommend shifting the scope to opting in to wide and full alignments, and leave left, center, and right off for this block (similar to current behaviour for the Heading, Post Title, Query Title, and Site Tagline blocks), but preserve the migration from left/center/right align values to textAlign values.
From a quick look at #32507, it looks like the main request was for wide alignment, so hopefully that'll still help improve the consistency for that + full width?
| <blockquote class="wp-block-quote has-text-align-right"><!-- wp:paragraph --> | ||
| <blockquote class="wp-block-quote alignright"><!-- wp:paragraph --> |
There was a problem hiding this comment.
This changes the output of the deprecation, which means that for a user who opens up this older markup, what once was a setting to align text to the right now sets the overall block to be aligned right.
There was a problem hiding this comment.
This was not a manual change, I ran the script to update the fixtures - so the error is in the code for the deprecation, correct?
There was a problem hiding this comment.
so the error is in the code for the deprecation, correct?
Yes, if we make an update to the deprecations code that results in having to regenerate the serialized version, then it's a good signal that we might have to make a change to the logic in the deprecated.js file. For this case, a good indication that the deprecation code is working correctly is if the existing core__quote__deprecated-2.serialized.html markup doesn't require any changes to be made in this PR. Ideally, the output of all existing deprecations should remain unchanged.
| save( { attributes } ) { | ||
| const { textAlign, value, citation } = attributes; | ||
|
|
||
| const className = classnames( { | ||
| [ `has-text-align-${ textAlign }` ]: textAlign, | ||
| } ); | ||
|
|
||
| return ( | ||
| <blockquote { ...useBlockProps.save( { className } ) }> | ||
| <RichText.Content multiline value={ value } /> | ||
| { ! RichText.isEmpty( citation ) && ( | ||
| <RichText.Content tagName="cite" value={ citation } /> | ||
| ) } | ||
| </blockquote> | ||
| ); | ||
| }, |
There was a problem hiding this comment.
I believe this save function should be a snapshot of the state of the save function prior to the changes in this PR. I.e. if we switch this version back to using align for generating has-text-align-${ align }, then it should hopefully get the core__quote__deprecated-2.serialized.html fixture passing again without that fixture needing to be changed.
|
Flaky tests detected in 737dda7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8004117759
|
There was a problem hiding this comment.
@carolinan, I tested the changes on both mobile and the web, and can confirm I didn't spot any functional regressions. I also created a companion PR in the Gutenberg Mobile repo, to verify that the unit tests pass there: wordpress-mobile/gutenberg-mobile#6267.
As I don't often work with the web, and given @aaronrobertshaw's previous involvement, I'd be happy to approve after he or someone else from the web team takes another look. Thanks!
There was a problem hiding this comment.
Thanks for all the hard work and effort that has gone into wrangling this one @carolinan 👍
There are still a couple of issues with the deprecations that we should probably iron out.
There's a lot of double handling of migrated attributes and inner blocks. If we made the (excluding migrateToQuoteV2) migration functions match the migrate signature, we could chain things together more.
As you can appreciate, deprecations are hard enough to grok without making the code harder to read too.
Changing the signatures to match migrate would also make it easier to update all the existing deprecations when new ones are needed.
The v2 migration of the large style made me do a double take as it didn't return the normal [attributes, innerBlocks] tuple. While it passes, I'd suggest we tweak that too.
Lastly, that v2 migration now migrates the classname in the attributes "out of order". The className attribute gets set after everything has already been migrated to v4 attributes.
Diff with suggested changes
diff --git a/packages/block-library/src/quote/deprecated.js b/packages/block-library/src/quote/deprecated.js
index a40eeb894b..fcf6776b76 100644
--- a/packages/block-library/src/quote/deprecated.js
+++ b/packages/block-library/src/quote/deprecated.js
@@ -37,13 +37,28 @@ export const migrateToQuoteV2 = ( attributes ) => {
const TEXT_ALIGN_OPTIONS = [ 'left', 'right', 'center' ];
// Migrate existing text alignment settings to the renamed attribute.
-const migrateTextAlign = ( attributes ) => {
+const migrateTextAlign = ( attributes, innerBlocks ) => {
const { align, ...rest } = attributes;
// Check if there are valid alignments stored in the old attribute
// and assign them to the new attribute name.
- return TEXT_ALIGN_OPTIONS.includes( align )
+ const migratedAttributes = TEXT_ALIGN_OPTIONS.includes( align )
? { ...rest, textAlign: align }
: attributes;
+
+ return [ migratedAttributes, innerBlocks ];
+};
+
+// Migrate the v2 blocks with style === `2`;
+const migrateLargeStyle = ( attributes, innerBlocks ) => {
+ return [
+ {
+ ...attributes,
+ className: attributes.className
+ ? attributes.className + ' is-style-large'
+ : 'is-style-large',
+ },
+ innerBlocks,
+ ];
};
// Version before the 'align' attribute was replaced with 'textAlign'.
@@ -169,9 +184,7 @@ const v3 = {
);
},
migrate( attributes ) {
- const [ v3Attributes, innerBlocks ] = migrateToQuoteV2( attributes );
- const v4Attributes = migrateTextAlign( v3Attributes );
- return [ v4Attributes, innerBlocks ];
+ return migrateTextAlign( ...migrateToQuoteV2( attributes ) );
},
};
@@ -195,9 +208,7 @@ const v2 = {
},
},
migrate( attributes ) {
- const [ v2Attributes, innerBlocks ] = migrateToQuoteV2( attributes );
- const v4Attributes = migrateTextAlign( v2Attributes );
- return [ v4Attributes, innerBlocks ];
+ return migrateTextAlign( ...migrateToQuoteV2( attributes ) );
},
save( { attributes } ) {
const { align, value, citation } = attributes;
@@ -240,22 +251,12 @@ const v1 = {
migrate( attributes ) {
if ( attributes.style === 2 ) {
const { style, ...restAttributes } = attributes;
- const [ v1Attributes, innerBlocks ] =
- migrateToQuoteV2( restAttributes );
- const v4Attributes = migrateTextAlign( v1Attributes );
-
- return {
- v4Attributes,
- innerBlocks,
- className: attributes.className
- ? attributes.className + ' is-style-large'
- : 'is-style-large',
- };
+ return migrateTextAlign(
+ ...migrateLargeStyle( ...migrateToQuoteV2( restAttributes ) )
+ );
}
- const [ v1Attributes, innerBlocks ] = migrateToQuoteV2( attributes );
- const v4Attributes = migrateTextAlign( v1Attributes );
- return [ v4Attributes, innerBlocks ];
+ return migrateTextAlign( ...migrateToQuoteV2( attributes ) );
},
save( { attributes } ) {
@@ -302,15 +303,10 @@ const v0 = {
migrate( attributes ) {
if ( ! isNaN( parseInt( attributes.style ) ) ) {
const { style, ...restAttributes } = attributes;
- const [ v0Attributes, innerBlocks ] =
- migrateToQuoteV2( restAttributes );
- const v4Attributes = migrateTextAlign( v0Attributes );
- return [ v4Attributes, innerBlocks ];
+ return migrateTextAlign( ...migrateToQuoteV2( restAttributes ) );
}
- const [ v0Attributes, innerBlocks ] = migrateToQuoteV2( attributes );
- const v4Attributes = migrateTextAlign( v0Attributes );
- return [ v4Attributes, innerBlocks ];
+ return migrateTextAlign( ...migrateToQuoteV2( attributes ) );
},
save( { attributes } ) {
diff --git a/packages/block-library/src/quote/edit.js b/packages/block-library/src/quote/edit.js
index 44370ee44d..4689899c6a 100644
--- a/packages/block-library/src/quote/edit.js
+++ b/packages/block-library/src/quote/edit.js
@@ -132,9 +132,7 @@ export default function QuoteEdit( {
createBlock( getDefaultBlockName() )
)
}
- { ...( ! isWebPlatform
- ? { textAlign: attributes.textAlign }
- : {} ) }
+ { ...( ! isWebPlatform ? { textAlign } : {} ) }
/>
) }
</BlockQuotation>
|
I was not able to refactor migrateTextAlign as you suggested in August, so I am not sure I will be able to do this, but I will look at your suggested code changes. |
FWIW I tested those suggested updates before adding them to the review. All the deprecation fixture tests passed locally here. Update: Just retested and tests still pass for me. The steps I took were:
|
SiobhyB
removed
the
Mobile App - Automation
There was a problem hiding this comment.
Nice work seeing this one across the line @carolinan 👍
✅ Tests as advertised
✅ Fixture tests and deprecations LGTM
From the web side of things I think this is ready. It also appears from this comment to be ready from the mobile side too but you might want to reconfirm with @SiobhyB.
|
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. |
|
Hi @WordPress/native-mobile How can we finalize this change? |
SiobhyB
added
the
Mobile App - Automation
|
@carolinan, I'm just starting work on reviewing now. :) |
There was a problem hiding this comment.
Approved from the native side! I manually tested to verify that the changes don't break the quote block in the app. I also created a Gutenberg Mobile PR to verify all automated tests pass for mobile here: wordpress-mobile/gutenberg-mobile#6640.
Thanks for looping in the native team here @carolinan!
| @@ -98,7 +98,7 @@ function BlockListItemContent( { | |||
| const name = getBlockName( clientId ); | |||
| const parentName = getBlockName( rootClientId ); | |||
| const { align } = getBlockAttributes( clientId ) || {}; | |||
| const { align: parentBlockAlign } = | |||
| const { textAlign: parentBlockAlign } = | |||
There was a problem hiding this comment.
@carolinan, thinking on this more, I realised we may need to co-ordinate releases to ensure centred quote blocks don't break for app users. Would you be able to hold off from merging until I co-ordinate a release plan with the mobile team? I'll update here when we have something in place. Thank you!
There was a problem hiding this comment.
@carolinan, thank you for your patience here! The earliest app release we'd be able to get this into is 24.4, which is set to be released to users on Monday, 18th March. With that in mind, could you merge this after the 17.8 Gutenberg release is cut this coming Wednesday? If this is included in the 17.9 release (set to be released on March 13th), it'd minimise the number of days it's not available for app users.
From my testing, I confirmed that having the change in place on the web but not in the app won't break the site’s content. The only issue I found is that the app's in-editor preview won’t reflect whether a Quote block is centred or right-aligned, which might be confusing. However, this will only be an issue for blocks that are first saved on the web then opened in the app. The in-app alignment buttons will continue to work and correct alignment will still be visible on the site’s front-end.
With the above in mind, we think minimising the number of days that the change is available on one platform but not another is the best option. Let me know if you're okay with delaying merge until after this Wednesday, for the 17.9 release. Thanks!
There was a problem hiding this comment.
Yes, it can wait.
There was a problem hiding this comment.
@carolinan, just to follow up, if you're able to merge this before this coming Thursday, we'll then work to get it into the app's 24.4 release. Thanks again for all your help here!
What?
Updated September 27 2023.
Renames the align attribute to textAlign.
Partial for #34454
There may be other blocks where the attribute should be renamed so the issue can be kept open until that is confirmed and solved.
Why?
The block used the
alignattribute for the text alignment.alignfor the block alignment andtextAlignfor text alignment.How?
Testing Instructions
Updated September 27 2023.
Screenshots or screencast
Before:
After:
