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.
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
Quote: Rename the align attribute to textAlign #42960
Quote: Rename the align attribute to textAlign #42960
Changes from 6 commits
6edad09
45a9863
746c4fb
745786e
976d99f
67d16e7
11ad22b
9112eda
2def1cc
ef50128
5fd2a25
a8057ff
757a999
beb1c9d
46a7b01
686ead6
6367fe6
215d6fc
51e1813
547539d
420beba
46940c5
2aa7d2e
ac28077
fa4566b
f2f96c5
48ef842
844482d
e1d245a
54188a1
92a9b73
f3ef6e7
8434f65
c45e17a
6509edd
ecd78fc
e280bac
f6a6f48
49d93d5
79d760c
57b74b7
7b1be6f
6245a17
1b76aba
7eae1e8
737dda7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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 generatinghas-text-align-${ align }
, then it should hopefully get thecore__quote__deprecated-2.serialized.html
fixture passing again without that fixture needing to be changed.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 doesn't seem right. The final html should have the new attribute..
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.
There is something wrong with the deprecation still. @andrewserong suggested (above) that the fixture files should be unchanged.
I have not manually changed them, I ran the command
npm fixtures:regenerate
because of the reported test failure.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.
Yes, that seems like it might be the case.
I think I might have been wrong about that, apologies, I think I might have missed some nuance in earlier reviews. In principle the block markup of this fixture should be unchanged (that is, we'd expect the HTML to be
<blockquote class="wp-block-quote has-text-align-right">
), but we'd also expect the block attributes in the block comment to have a value fortextAlign
in the serialized version.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.
Who is the best person to ping to review the deprecation?
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.
It could just be me but the adoption of the
align
block support at the same time as this renaming/deprecation makes it harder to reason about.That said, I agree, the results of this deprecation fixture aren't correct. As @andrewserong noted, the text align class should still be present and the attribute in the bloc comment delimiter updated to
textAlign
if it was migrated properly.Fixing
migrateTextAlign
, or its wrapping ofmigrateToQuoteV2
, gets this fixture to fail due to the incorrect block attributes.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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 existingcore__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.