Block variations transformations #26687
Conversation
ntsekouras
added
[Package] Block editor
ntsekouras
requested review from
draganescu,
youknowriad,
noisysocks and
gziolo
|
Size Change: +1.47 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
docs/designers-developers/developers/block-api/block-registration.md
Outdated
Show resolved
Hide resolved
docs/designers-developers/developers/block-api/block-registration.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-variation-transforms/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-variation-transforms/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-variation-transforms/index.js
Outdated
Show resolved
Hide resolved
docs/designers-developers/developers/block-api/block-registration.md
Outdated
Show resolved
Hide resolved
|
This is what I see:
An overview: I have a few comments that is outside the scope of this PR, but they show up as a natural extension of adding the Variation drop down. We see text for selecting horizontal or vertical. Which makes it pretty clear what it does. At a later time one will likely think of other Navigation variations that can be added to the drop down. Variations where it would be beneficial to use text and icons. I am wondering if we should in general use text and icons for all variations.
--
|
Maybe it would be better just to hide it during initialisation.. I'll have to check more how to make it work with every block best, as most of the blocks do not have a Placeholder for creation. I think it'll become more obvious after checking what core blocks will use that feature. I'll do that after we have some design in place for the available options.
I agree with that. Icons will be pretty useful. |
|
I wonder, probably silly, why do variations need to opt in to be available as transforms? Can't we default to having all variations available? |
|
Hey @draganescu!
There are more nuances to this, especially when having variations with InnerBlocks just like Columns and Query that we have to handle the content as well and probably have to do it manually with some kind of For example when we have a |
ntsekouras
force-pushed
the
try/transform-block-variations
branch
from
f1b9a1f to
1b142b3
Compare
There was a problem hiding this comment.
Code wise, this PR is ready to go. There are two things that need to happen to consider it done:
- documentation for
BlockVariationTransformscomponent needs to be polished - design review is required for the first iteration
I will leave the final call to other folks but from myself it's ✅
ntsekouras
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz and
Soean
as code owners
packages/block-editor/src/components/block-variation-transforms/README.md
Show resolved
Hide resolved
|
I believe it will benefit the user to use thumbnails for all variations. As one would get text and a visual image. It would keep a consistency in relation to the UI for variations that all use thumbnails. As I mention in this comment: #25231 (comment) In relation to the Navigation variation drop down in this PR. Without thumbnails: With thumbnails: In relation to the horizontal and vertical variation having a thumbnail might not be that important, but if one begins to add other variations to the Navigation block then thumbnails might be needed to give a visual impression. Picking and choosing when to use thumbnails and not would break the consistency. Which means even though some variations are easy to understand with the text it would be better to have all variations using a thumbnail approach. Somewhat similar to the Styles Panel. Some people understand better with visual and some better with text, using both can be very helpful. |
|
Question: why does the navigation block appear as two blocks, requiring an entirely new UI to transform between them, when we already have this?
This seems like a lot of added complexity, for reasons I can't really spot. |
ntsekouras
force-pushed
the
try/transform-block-variations
branch
from
4247673 to
3f41afb
Compare
|
This is what I see in the latest iteration:
Seems like there's an opportunity in the future to revisit all transforms (Shaun has lots of good thoughts). But in the mean time, and in the spirit of iterating based on based on this feedback, there are a couple of things: The dropdown should show the selected variation when collapsed. I.e. instead of saying "Transform to variation", it should say "Horizontal" or "Vertical". There's something up with the padding, it's too tight:
It's this CSS that overrides it:
Here it is with its intrinsic padding:
That chevron on the right still looks funky and scaled down, but that looks like a separate issue with the dropdown component itself. There doesn't seem to be a default choice. I.e. when I insert a horizontal navigation, I'd expect the checkmark to be present for the horizontal choice. The menu jumps a bit in width, which is jarring. You probably need to provide a min-width to the popover. Finally, instead of saying "Navigation (horizontal)", should it just say "Horizontal"? |
|
Please note in the above GIF, the actual variation doesn't appear to change in the content. That's the fault of my theme. It works fine in TwentyTwentyOne |
|
Thanks for reviewing @jasmussen!
This is a problem quite difficult/expensive to solve and with no big value, especially when this will include transformations with
This change would make the Component difficult to understand what is showing, no? We have no other label. I do believe though that's fine because in combination with the above comment of not knowing the variation exactly (it will be quite obvious with the InnerBlocks variations) it could be not valid information as well.
This is the block variation's description... |
Is there a hack? Showing whichever variation of block you chose, without it actually being "set"? For example with Social Links, the default size is "Medium". It hasn't actually been set, and you can choose to explicitly set it, which adds the classname. But we still show a checkmark next to medium, because that's the size you see.
I'm just thinking of this in terms of being a dropdown menu — a dropdown menu should show the selected choice. Anything else is a bit of a departure from the design, and if we can't make it work as such, I'm not sure it's as effective. |
packages/block-editor/src/components/block-variation-transforms/README.md
Outdated
Show resolved
Hide resolved
Yeah, that is handled with default css and can't work in a generic component like this.
I'm not sure what other alternatives we have here. I think this component is more like an If we remove the |
I second that idea. The issue with the current proposal is that it will work as intended only when a user triggers the transformation in the dropdown menu. As soon the user navigates away from the visual editor, by opening a code editor mode or going to another page, there is no simple way to detect which transformation was applied. Well, in this particular case, it would be easy to detect, because there is only one attribute that gets updated. However, as @ntsekouras already mentioned, this can quickly become complex to verify when there are inner blocks involved. Regardless, we would need to extend block variations API with a logic that detects the currently applied transform if any. I can imagine a case when variations don't cover all combinations of values that attribute to be modified can cover. Aside: rather than using block variations for the Navigation block, we could also add the same dropdown that modified attributes directly ... 😃 |
|
Part of the problem is that this is yet another way to transform blocks. I understand it's meant to solve this problem:
In light of that, it seemed like like this dropdown design was a decent path forward — relatively minimal but provided a path forward. But that design is inherently a dropdown, so if it doesn't show the current variation in its resting state, the design falls apart a bit. Pinging a few for some more thoughts: @shaunandrews @kellychoffman |
Since we can revisit later when more complex transforms will be handled, another alternative is to check the selected variation, if there is a match, by checking the attributes for now. That means that this will be the same as now but will have the matched variation with the I have to note again though that this matching might not be possible/valid with complex variations later. |
|
I had also this idea that I shared with @ntsekouras in a private message. If we assume that those transforms never use the inner blocks template but rather call a callback to apply changes, we could easily detect whether any block variation matches current attributes. There might be a limitation where only one variation could match and it would need at least one attribute provided. In fact, the case we have fits perfectly fine with the description provided. It might not work well in another case, but we better battle test it first rather than overthink. |
| @@ -0,0 +1,38 @@ | |||
| .block-editor-block-variation-transforms { | |||
There was a problem hiding this comment.
It looks like a lot of hassle to style the toggle button to look like something completely different.
Do we have other components that are customized to look and behave like a select control that allows custom HTML code for items? The one that allows picking a different font size might be a better fit in terms of styling and accessibility support.
It's something to think about, the current proposal is fine to test this feature as an experiment.
There was a problem hiding this comment.
I have checked that (CustomSelectControl) but would want many more additions to support better handling of content - it's not so generic for now.. I think for now it's okay and we can revisit whether to enhance the CustomSelect or create something else.
There was a problem hiding this comment.
CustomSelectControl is a tiny wrapper over downshift.js which isn't opinionated about HTML used:
I would be very surprised if you wouldn't be able to use it here, but I agree that you might need to apply some tweaks as well. The benefit is that it better reflects its purpose from the accessibility point of view.
There was a problem hiding this comment.
I started with that, but saw that needed enough tweaks and seemed rather specific contrary to its name.
- You think I should change it now or in a follow up PR?
- The
DropdownMenusuffers from accessibility point of view? Should we look how to improve this as well? (I haven't dig into accessibility yet)
packages/block-editor/src/components/block-variation-transforms/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-variation-transforms/index.js
Outdated
Show resolved
Hide resolved
Description
Resolves: #25231
This PR is the first step for handling block variation transformations. It introduces a
transformnew option forscopefield in block variations. This way we can control how and when to handle these transformations, as this behavior doesn't always make sense for all variations for every block.In this PR we support block variations that change only attributes, like the
Navigationblock (which is the only one adjusted for testing reasons). The design is not finalized yet and I've implemented it with a simple select control for now, just to test how this feels.The second step in a follow up PR will be to handle transformations with
InnerBlockslike Query or Columns that will need an API to manually handle such block variation transformations. For example what happens when we have Columns (4) and we want to expose the transform to Columns (3) etc...Testing Instructions
Checklist: