Block variations: Add block-supports flag to add variation specific classname #61864
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
tjcafferkey
commented
May 22, 2024
|
Size Change: +222 B (+0.01%) Total Size: 1.76 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
ockham
requested changes
May 27, 2024
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
ntsekouras
reviewed
Jun 10, 2024
ockham
previously approved these changes
Jun 10, 2024
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
mcsf
reviewed
Jun 10, 2024
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. |
This reverts commit 7011264.
instead of comparing it to `true` or `false` Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
ockham
force-pushed
the
add/block-variation-classname
branch
from
f1b7b03
to
4c1a12b
Compare
|
Rebased. |
16 tasks
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.
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