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
Block variations: Add block-supports flag to add variation specific classname #61864
base: trunk
Are you sure you want to change the base?
Block variations: Add block-supports flag to add variation specific classname #61864
Changes from all commits
6fe71ec
d42c4d1
1081fe4
6cd7199
608bfd6
d9036b6
4de031b
b1bd36c
3cb6023
e384ec7
5dce388
e99838e
cb9430a
b47cc1f
2bde392
1568b02
818af29
b34a14e
8515de7
2df5db7
d625240
faac58c
c8b9a6a
2b215d7
0af2def
962a5ae
173bef2
5cccab7
cff8fcc
50a0044
0d34163
aaaa855
e819f54
7cf6f8e
6ad4153
bc3fb00
a37f5d3
25f7a71
6f016b3
d369a97
7350ced
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.
Would this be equivalent with
hasBlockSupport( nameOrType, 'className.block', true );
?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 think so, yeah. Are you suggesting we replace it with that? 🙂
(Personally, I find it a bit easier to read if we keep it as-is: We've fetched the value of
className
block-support and have determined that it's not a boolean, so now we check if it has ablock
property and return its value if it does; otherwise, we default totrue
. It also removes the need for separately calling thehasBlockSupport
selector, since we already have fetchedclassNameSupport
.)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.
Should we default to
true
for this feature? Have you discussed this? This would also affect the default handling when we havesupports.className: true
.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 was originally hoping to do that (and discussed it with @gziolo again today), but I've come to the conclusion that it's too much of a risk. As you mentioned elsewhere, setting this to
true
will change the serialized markup for existing blocks, so we'll need to add migrations for them.Having to add those for any block with variations that wants to use this new feature seemed like a bit of a hassle at first, but I now think it's an okay tradeoff. It's probably worse for plugins (and authors) that include a large number of blocks -- and for Core itself -- but even then, not all blocks come with variations, and it's a one time change. IMO, it's reasonable to ask extenders to enable this feature manually (and to add a deprecation) in exchange for a useful enhancement.
Since we might also need a server-side part (which will require a robust way of determining the active variation), it makes even more sense to require extenders to opt into this new feature explicitly, as it gives them control over adding this new class name, and allows them to fix their
isActive
fields before they do so.