Quote v2: update how attributes are registered #39729
Merged
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
oandregal
added
[Type] Bug
oandregal
changed the title
|
Size Change: +64 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
This was referenced Mar 24, 2022
oandregal
force-pushed
the
update/quote-block-v2-register
branch
from
f2b736f to
49595f4
Compare
oandregal
force-pushed
the
update/quote-block-v2-register
branch
from
49595f4 to
aa5f957
Compare
youknowriad
reviewed
Mar 25, 2022
| selector: 'figcaption', | ||
| default: '', | ||
| __experimentalRole: 'content', | ||
| }, |
There was a problem hiding this comment.
What would be the issue if we use the same block.json file for both versions. I mean aside having all the old attributes in the version, which doesn't bother me much specially since we need to keep them to implement the "migration effect" behavior.
There was a problem hiding this comment.
It was the other way around I was concerned about: I didn't want to pollute the existing quote block with the v2 attributes.
Technically, it might work in this case, as we're not modifying existing atts but adding new ones. I can't think of anything that would break. Though I still not a fan of this approach, I rather keep things separated.
There was a problem hiding this comment.
Ok, let's keep them separated for now, it doesn't hurt either.
youknowriad
reviewed
Mar 25, 2022
| @@ -29,7 +29,8 @@ | |||
| "src/**/*.scss", | |||
| "src/navigation-link/index.js", | |||
| "src/template-part/index.js", | |||
| "src/query/index.js" | |||
| "src/query/index.js", | |||
| "src/quote/v2/index.js" | |||
There was a problem hiding this comment.
Curious about this change, how do we decide to add a file here.
There was a problem hiding this comment.
My understanding is that we use this to signal webpack not to remove things that aren't part of the export in those files. In theory, it should remove the addFilter call. In practice, though, I tested without adding this and things worked as expected. Not sure if there's some webpack config missing, but wanted to play it safe and avoid the situation in which the v2 block stops working because of some webpack config changed.
youknowriad
approved these changes
Mar 25, 2022
There was a problem hiding this comment.
I think it's probably unrelated with this PR particularly but there's weird behavior with the quote block where if I toggle "add attribution" in the toolbar, I'm not able to see the toolbar again if I select the quote block. (Probably something wrong because we're changing the block wrapper)
Also, I noticed that in 2022, both the quote and the paragraph have a left border. (I guess due to the markup change).
Anyway, I can approve this PR (the attributes change) but would be good to check the two points above separately or track them.
|
Yeah, I also saw some other weird things with margins. We should review styling before making v2 the default.
Did you have the focus in the attribution or within the quote (paragraph or so)? This is a confusing interaction for me as well. It's hard to see the quote toolbar if you don't have a attribution to get the focus into. I've marked that as a task at #25892 (comment) |
|
I'm going to merge this to unblock #39718 |
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 to #39703 and #39704
What?
This PR updates how the new attribute for Quote v2 is registered. As a side-effect, the v2 quote block declares all the block supports the v1 has and the
alignattribute as well.Why?
Using a separate
block.jsonfor v2 is problematic because it's not picked up automatically.How?
Instead of having a separate
block.jsonfor v2, this uses theblocks.registerBlockTypehook to register the new attributeattributionand unregister the old onecitation.Testing Instructions
Check that attributes are registered properly is v2 is active:
core/blocksstore contains the proper block attributes underblockTypes.core/quote:valueandcitationattributesattributionattributeCheck that attributes are registered properly if v1 is active:
core/blocksstore contains the proper block attributes underblockTypes.core/quote:valueandcitationattributesattributionattribute