Block variations: Add block-supports flag to add variation specific classname #61864
Conversation
|
Size Change: +1.79 kB (+0.1%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
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. |
tjcafferkey
added
[Feature] Block API
There was a problem hiding this comment.
I think the code that adds the variation class name should be colocated with the code that adds the auto-generated block class name 🤔 The latter is only added conditionally to the block if it has className block-supports.
The relevant code is encapsulated in generated-class-name.js, so it looks like adding the variation class name there would also preserve parity with what we're doing on the server.
I'd echo that. We should inject the class name and it should be auto generated. We shouldn't preserve the className in the html markup as it will create various issues if we switch to a different variation. |
I agree with colocating the code, but AFAIK the code in |
|
👋 I've only now started reviewing this, and I think it'll take me a bit longer to let it all sink in. Coincidentally, I came across #56469 today, which changed the List block -- which previously had That PR is interesting -- and potentially relevant to the problem we're trying to solve. More surprisingly to me, the PR adds a server-side gutenberg/packages/block-library/src/list/index.php Lines 9 to 11 in d80b5a9 ...and possibly in a prior discussion on a GH issue:
Some ideas from that other PR might help us reduce some of the more "magical" parts from our approach. Anyway, I'll need a bit more time to fully wrap my head around this. |
|
Another observation: If I revert 83ddbfb (i.e. #56469), the
|
ockham
force-pushed
the
add/block-variation-classname
branch
2 times, most recently
from
ae6c466
to
cd945bd
Compare
ockham
dismissed
their stale review
Just wanted to retract my Change request, didn't want to approve quite yet.
|
I'll rebase. |
ockham
force-pushed
the
add/block-variation-classname
branch
2 times, most recently
from
dee14ff
to
6bbb713
Compare
packages/block-editor/src/hooks/generated-variation-class-name.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/hooks/generated-variation-class-name.js
Outdated
Show resolved
Hide resolved
|
Just noting some thoughts / discoveries down here:
|
This means that any modification and subsequent saving of the template will cause the variation className to be added, right? If that's the case, then it's pretty much the behavior I'd expect 👍
I understand that's fixed now? 😄 |
|
When a template is loaded into the Visual Editor, variation classNames are added to existing static blocks (visible through DevTools by inspecting the DOM). However, if you switch immediately to the Code Editor, the block markup will appear without these classNames. Making any change in the Visual Editor will then force the variation classNames to appear in the Code Editor. These new classNames will not show on the frontend until you save your template. Initially, I thought this discrepancy between the Visual and Code Editors needed fixing, but this behavior is already present in the deprecation/migration API. For example, when deprecating a block to change its tag name, the Visual Editor shows the new tag name in the DOM, while the old tag name appears in the Code Editor until the user makes a change and saves the post/template. |
tjcafferkey
changed the title
This reverts commit 7011264.
instead of comparing it to `true` or `false` Co-authored-by: Nik Tsekouras <[email protected]>
Co-authored-by: Nik Tsekouras <[email protected]>
Co-authored-by: Greg Ziółkowski <[email protected]>
ockham
force-pushed
the
add/block-variation-classname
branch
from
611dd12
to
6f016b3
Compare
Indeed. I've replied to your comment in #61864 (comment) and #61864 (comment).
Yeah, it would invalidate them. If you look at the patch that's part of the testing instructions in the PR description, it's three lines to opt the Group block into the auto-generated variation-specific class name, and another ~100 lines to add the deprecation 😬 |
Yeah, I'd also love to get this one done; it's been open for a couple of months now. I have some concerns around committing to a format for both the block-supports semantics I've chosen (especially since they're non-experimental), the usage of the Furthermore, I think there's a real chance that folks will want us to add the variation class names on the server side as well (as Nick pointed out); if people feel fairly strongly about this, then we should probably not get the client-side part into 6.7 without the server-side part (which is unfortunately a bit more complex to get right, as discussed). So timing is a bit tricky here 😕 |
I'm not worried about the semantics of I'm starting to think that we should unbundle the variations class name generation from the default class name, example implementation: function generateDefaultClassName( blockName, variationName = '' ) {
// Generated HTML classes for blocks follow the `wp-block-{name}` nomenclature.
// Blocks provided by WordPress drop the prefixes 'core/' or 'core-' (historically used in 'core-embed/').
const className = 'wp-block-' + blockName.replace( /\//, '-' ).replace( /^core-/, '' );
return variationName? className + '__' + variationName : className;
}
export function getBlockDefaultClassName( blockName ) {
const className = generateDefaultClassName( blockName );
return applyFilters(
'blocks.getBlockDefaultClassName',
className,
blockName
);
}
// This one is more nuanced, but the simplified version is to illustrate the idea.
export function getBlockDefaultVariationClassName( blockName, variationName ) {
const className = generateDefaultClassName( blockName, variationName );
return applyFilters(
'blocks.getBlockDefaultVariationClassName',
className,
blockName,
variationName
);
}This way, we would start fresh for block variations. In case, folks don't like the pattern used when generating the class name for block variation they have another filter to use in addition to preexisting |
Yeah, I think that's a good idea. As discussed on our call earlier this week, I'll pick this up after GB 19.3 RC1 is out (which is due today; however, I'll be gone for a week, starting tomorrow). I will try to land it soon after that, though. This way, it won't be in WP 6.7, and we'll have a bit more time to tweak it. |
Related to: #43743
What?
For a given variation of a static block that is registered it will add a unique className specific to the variation to the block markup. For example
group-rowwhich is a variant ofcore/groupwould have the resulting CSS className:wp-block-group wp-block-group-group-row.Why?
As part of the project to support block aliases for variations (tracking issue) one of the requirements is to provide an additional CSS className to provide more styling options for these blocks.
How?
We chose to update the existing
classNameblock-support flag. Previously, settingclassName: truewould cause the default block classname (wp-block-my-block) to be added. This continues to be the default value and behavior for that flag.Additionally, the following syntax is now supported:
The
blockproperty controls adding the default block classname, whereas thevariationproperty is in charge of adding the variation-specific classname (wp-block-my-variation).Question: Are we happy with the choice of names for these properties (
block,variation)? Should they be called differently?Testing Instructions
wp-block-group-group-row).Patch
Screenshot