-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Details Block: Rmove unnecessary comment attributes #51610
Conversation
Size Change: +75 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
Flaky tests detected in d9d4f23. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5306943596
|
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.
Thanks for putting this PR together @t-hamano 👍
For the deprecation to work properly, we'll need to also provide the block supports configuration for the v0
deprecation. Otherwise, if there was a deprecated version of the block with block support style attributes set, we'd lose that by the time it was migrated.
In addition, there are some further changes to the block fixtures required.
- The default fixture (i.e. for the latest version) should have the source
core__details.html
updated to remove the summary from the block comment delimiter. - There should be a new fixture added to cover this deprecated case e.g.
core__details__summary__deprecated.html
. Essentially, its source HTML file would be the same as thecore__details.html
stands now (with the summary value in the block comment delimiter).
Thanks for the review, @aaronrobertshaw! I added the |
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.
Thanks for iterating on this one @t-hamano 👍
The deprecation's supports now match the block's, the fixture tweaks look good, and the fixture tests are all passing.
We might need to rebase this one though before merging.
Thanks for the review, @aaronrobertshaw! I found two things to worry about after merging the latest trunk.
999179be3df23939c2957a5055ff85a4.mp4 |
Changes to block supports would only need to trigger a migration if they impact the saved markup for the block. In this case, the support changes are only to control what UI controls are displayed by default. Essentially, This PR is introducing the first deprecation for the Details block. It should reflect the current state of the attributes and supports at the time it lands. This would include the
If I'm following correctly, the question is why doesn't the block migrate without editing it? My guess would be that the block markup is still valid as it is only the comment delimiter that is changing. This means the block isn't invalidated. Otherwise, valid blocks can still trigger a deprecation to run if their I quickly hacked up something locally but ran out of time to get it working properly. The snippet below does not appear to be updating the block delimiter despite clearing the summary attribute. It also then incorrectly shows the "Write summary" placeholder. Unfortunately, I'm out of time this week so might have to leave this one with you. diff --git a/packages/block-library/src/details/deprecated.js b/packages/block-library/src/details/deprecated.js
index e64bf11ce16..35181fa1588 100644
--- a/packages/block-library/src/details/deprecated.js
+++ b/packages/block-library/src/details/deprecated.js
@@ -48,6 +48,12 @@ const v0 = {
},
},
},
+ migrate: ( attributes, innerBlocks ) => [
+ { ...attributes, summary: undefined },
+ innerBlocks,
+ ],
+ isEligible: ( attributes, innerBlocks, { blockNode } ) =>
+ !! blockNode.attrs?.summary,
save( { attributes } ) {
const { showContent } = attributes;
const summary = attributes.summary ? attributes.summary : 'Details';
Having the summary attribute within the block delimiter doesn't break things. Any new details block, or any existing one that gets updated, will have the attribute cleared from the block delimiter. Perhaps, we should avoid the deprecation altogether? Some fresh eyes and opinions on this would be welcome cc/ @andrewserong |
Thanks for the ping!
Yes, in this case because the block markup hasn't changed, my understanding is that we wouldn't need a deprecation here. I don't think there's a need for us to migrate old comment markup to the new format — as you mention, we can happily change what gets serialised when the content of the block is manually updated by the user, rather than forcing it via a deprecation. I like the idea of this PR, though, thanks for working on it @t-hamano! It seems better overall to me to not store a duplicate of the data in the comment delimiter 👍 |
Thank you for the detailed explanation, @aaronrobertshaw, @andrewserong! If I understand correctly, this PR only requires the addition of the 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.
Thanks for the updates @t-hamano!
This is testing nicely:
✅ When opening an existing post where summary
is stored in the comment delimiter, there are no errors or warnings
✅ After making any edit, the markup is updated to remove the summary
attribute in the comment
✅ Saving and reloading the editor doesn't result in any errors or warnings
Initial page load | After making an edit |
---|---|
LGTM! ✨
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.
Appreciate your patience on this one @t-hamano 🙇
I think we've settled on the right approach to this issue and it's testing well for me.
✅ Deprecated blocks are still valid
✅ Editing blocks updates the block comment delimiter
✅ New blocks omit summary from the comment delimiter as well
🚢
Thanks for the review, @aaronrobertshaw, @andrewserong! |
What?
This PR changes the attributes definition of the summary property so that content is not stored in the comment delimiter.
Why?
My understanding is that the values of attributes should be extracted from the markup as much as possible to reduce the amount of content stored in the database.
How?
Add a deprecation.Addselector
andsource
tosummary
attribute and updated the existing fixture.Testing Instructions
{"summary":"Details Summary"}
.When the editor is reloaded, confirm that the browser console outputs a log indicating that a block deprecation has been performed.Update the contents of the block.