Add flex layout to Buttons block and new layout type. #35819
Conversation
tellthemachines
requested review from
ajitbohra,
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
tellthemachines
requested review from
ntsekouras,
oandregal,
talldan and
noisysocks
tellthemachines
added
[Block] Buttons
|
Size Change: +299 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
Hey, I'm not sure if I can provide much help here. Riad, Joen, and Andrew Serong have been involved in a lot of this work (probably others too), so they'd probably have some thoughts hore. |
tellthemachines
requested review from
youknowriad,
jasmussen and
andrewserong
|
Thanks for the PR, took it for a spin. Just for comparison, here's a "before" GIF showing horizontal and vertical orientations with the justifications and spacing working: Here's an "after" gif, showing the same but from this branch:
Note one difference, here the justification controls are shown both in the toolbar and the inspector, which is the custom for blocks that opt into the proper flex method. That's good. So just as a user experience and behavior, this appears to be working as intended 👍 👍 One thing I discovered. which could be due to my local environment, but sharing in case not:
Here's the markup I tried, and it shows up as horizontal in this branch: |
|
Nice idea @tellthemachines! The general approach sounds good to me, I like that the resulting CSS for the Buttons block is essentially unchanged 👍. There were a bunch of edge cases that @stacimc had to wrangle in #34546, so this looks like it shouldn't affect those changes, which is good.
I think that sounds like it's because of the removal of the |
| }; | ||
|
|
||
| export default { | ||
| name: 'column', |
There was a problem hiding this comment.
Do you think this layout will have other use-cases aside the "buttons" one?
What happens when we apply this layout type to a group, does it bring any value? Do we want a variation like "Row" added for the group block with this layout?
There was a problem hiding this comment.
I think of this column layout as a subtype of flex and its usefulness lies on the justification (at least for now).
The default/flow layout is another way to have a vertical list of blocks which now handles the widths of the block and targets grouping blocks.
Could this justification option be integrated with the default layout? Would it make sense? That would mean changing the display of Group (and other blocks that use the default layout) to flex and I'm not sure if there would be implications with that.
There was a problem hiding this comment.
I think of this column layout as a subtype of flex and its usefulness lies on the justification (at least for now).
So potentially could also be implemented by introducing an "orientation" config to the "flex" layout itself.
There was a problem hiding this comment.
That's another option for sure and that was my first thoughts about that when working with flex layout.
There was a problem hiding this comment.
Thanks for the feedback! I've updated to use an "orientation" property on the flex layout. It makes sense because there was a lot of duplicated logic between flex and column.
|
Thanks for the feedback, everyone! I've addressed comments, added a deprecation and updated the fixtures so this should be ready for a more in-depth review now 🙂 |
tellthemachines
force-pushed
the
add/buttons-flex-layout
branch
from
2c9e22b
to
6cc5c75
Compare
tellthemachines
force-pushed
the
add/buttons-flex-layout
branch
from
6cc5c75
to
678d5d5
Compare
|
Took this for a spin. I noticed that when switching between buttons variations, I'm losing the "Justify config". It feels like we should probably keep it. In other words, an It seems there's only "space between" that is not present on the vertical variation, while right now it's true that it doesn't make sense but for me it would still make sense there if we say introduce a "height" control later for the buttons block. |
Yeah, I looked into this and it's happening because of how we're setting the orientation as a block variation. The block variation defines e.g.
If we do add a height control, then perhaps we could make "space-between" display conditionally on the height being set? Because otherwise it won't have any effect, and the experience will feel broken. |
tellthemachines
force-pushed
the
add/buttons-flex-layout
branch
from
678d5d5
to
9bf62af
Compare
|
I've updated this to set orientation in the Layout section of the block inspector, instead of as a block variation. I also made it so that only blocks that have "orientation" defined in their layout attribute show the orientation controls, because they might not make sense for some blocks (I'm thinking of the Row variation of Group that should specifically be horizontal, but perhaps with this change we could potentially stop using Row altogether?) |
|
Lovely work as usual. I see this segmented control in the inspector now:
I'll defer to others including Riad on whether an orientation toggle is the correct one, but I do recall @shaunandrews already suggesting one, which is one vote in favor. He's also brought up that we should mostly avoid segmented controls with only two options. I think we can do better than the following sketch — pretty sure Shaun actually has a mockup somewhere we can leverage. But in the name of landing the feature in the near term, such a toggle group with two arrows might be a good initial start that we can then polish further:
We might even be able to show the two controls on a single line:
|
| @@ -39,6 +52,12 @@ export default { | |||
| layout={ layout } | |||
| onChange={ onChange } | |||
| /> | |||
| { layout?.orientation && ( | |||
There was a problem hiding this comment.
This check is interesting. It basically means that if the current layout doesn't have any orientation, do not display the control to switch "orientation". That said one can argue that the "no orientation" is just equivalent to "horizontal orientation" meaning we could just always show the toggle.
The problem with always showing the toggle is that it will show up in other blocks using the "flex" layout like the "Group" block (or its Row variation and potentially others). While one could argue that this is wrong since is a "row" right, conceptually it's not wrong. For instance, what about blocks like "Social Icons" and "Query Pagination" also using the "flex" layout, should we allow them to switch orientation? It might make sense.
So the thing here is that I'm hesitant between:
- Keep things as they are in the PR: making the "orientation" undefined or not in the default layout value the actual flag that indicates whether to show this control or not.
- Introduce a dedicated opt-out flag in the layout support config (and potentially only use it in the Group block)
There was a problem hiding this comment.
An opt-out flag would be nice, especially if there's only one block where we really don't want to enable vertical orientation. Perhaps something like "allowOrientation": false?
It also occurred to me that if orientation were enabled for all blocks, we wouldn't need Row as a variation of Group at all. But Group is using flow layout; would there be any issues in migrating it to flex with vertical orientation?
There was a problem hiding this comment.
Well, a flexbox layout doesn't let children use float, so yes, there would definitely be issues if "flow" did not remain as an option.
There was a problem hiding this comment.
Oooh good point. I guess we'd better leave it as it is then.
|
I updated the inspector controls and tried switching orientation to opt out; it seems to work well! Any further comments, feedback etc.? |
There was a problem hiding this comment.
Thanks so much for your great work here @tellthemachines!
Sorry for being late catching up, but I've left some comments to discuss 😄
| } } | ||
| /> | ||
| </BlockControls> | ||
| <BlockControls |
| return attributes; | ||
| } | ||
|
|
||
| const { contentJustification, orientation } = attributes; |
There was a problem hiding this comment.
We should remove these attributes here and keep only the rest. You can also see them appear in the fixtures.
There was a problem hiding this comment.
I'm not sure I understand. These are used here to migrate any legacy settings to the new layout, e.g. if there's a deprecated block with "orientation" set we use that value in layout when we update the block. Or is there another way of doing it?
In the fixtures these attributes only appear in the deprecated versions of Buttons. That should be expected, right?
There was a problem hiding this comment.
Oh wait, you mean I should be removing them from the updated attributes?
There was a problem hiding this comment.
I should be removing them from the updated attributes?
Yes. We extract them here and use them for the layout but since the current version doesn't contain them, these attributes shouldn't exist at the final migrated version of the block.
| }, | ||
| }, | ||
| }, | ||
| isEligible: ( { layout } ) => ! layout, |
There was a problem hiding this comment.
I don't think we should have isEligible here. The thing is that when this function is checked it gets the attributes for this deprecation. This ends up having a newly created buttons block (using its default layout) and then in migrate the orientation:horizontal exists and applies the defaults from the migration.
You can check this by adding a different default in buttons block.json like:
"default": {
"type": "flex",
"orientation": "vertical",
"justifyContent": "right"
}
Add a Buttons block, save and then reload.
There was a problem hiding this comment.
Oooh, interesting. How does it work in the Social Links deprecation?
I can't remove isEligible altogether or the deprecation won't work, but I'll change the check to explicitly look for the contentJustification and orientation attributes, as these are the only ones we need to migrate. If they don't exist we can leave the block as is.
There was a problem hiding this comment.
All feedback has now been addressed in #36192. Thanks for the review!
There was a problem hiding this comment.
How does it work in the Social Links deprecation?
Because:
The specific handling by
classNamebelow is needed becauseitemsJustification
was introduced in https://github.com/WordPress/gutenberg/pull/28980/files and wasn't
declared in block.json.
So I didn't remove any attributes there.
|
Ha, @jasmussen here's the mockup I had from a few months back:
The biggest difference from what you posted about is the language. I find "Direction" and "Alignment" to be more easily understandable than "Orientation" and "Justification," which feel more like jargon. -- What I see in
I feel like the Orientation should be shown first, with the Justification settings after — and that these two controls are connected; If I change the Orientation to vertical, the Justification icons should rotate and their descriptions should update to use top and bottom instead of left and right. Similarly, I'm not sure if the "multiple lines" toggle makes much sense when the Orientation is set to vertical. Why doesn't the vertical orientation support space-between? I guess it would require the container to have a defined height? Here's how I would expect it to look:
|
|
One question: can we now remove the legacy alignment styles for Buttons or should they remain for back-compat purposes? |
I think we've tried a few times (@kjellr is so busy these days he might not have time to respond here), and we've always come back to not being able to. We might be in the future, but if you're able to keep them for the time being, it's probably for the best. |
|
Yeah, we've tried to remove those in the past, but they've broken existing content. I'd avoid it here if we can. |
|
Is it still possible to define contentJustification in our block templates? Did the syntax change? When I try to center a button within my code like this:
The blocks load initially but after publishing and refreshing the page in the admin, the buttons block breaks and I get the following error in the console:
|
|
The syntax for content justification did change; instead of Could you share some reproduction steps and the version of Gutenberg you're on? |
|
@tellthemachines unfortunately deprecations don't work for templates, this has always been the case in Gutenberg. It's not really clear what the solution forward for this is. It might require some structural or API changes ultimately. |
Description
Partially addresses #34872.
The Buttons block is slightly different to other blocks that have been migrated to layouts, because it has a Vertical variation that behaves in a flex column-like manner, and can be justified (though calling it justification is slightly inaccurate, because behind the scenes we're actually using
align-itemsinstead ofjustify-contentto make it work with the column layout).This means that neither of the current layout options will work here: flow doesn't have flex properties, and flex is optimised for horizontal layouts. So I introduced a new "column" layout variation that uses
flex-direction: columnand works with justification controls, translating them to usealign-items.I'd love to get some feedback on this approach before I start dealing with deprecations, tests etc. 😄
I left the second part of #34872 (adding flex layout to the Navigation Block) out for now, because though it has a similar Vertical variation, the way styles are applied to the content of the block is much more complex, as we have several nested containers that need to be configured with the orientation and justification/alignment flex properties. Layout currently works by applying styles generically to the top level of the block, so we'll have to think about how to make that work with nested containers. Or if we want to make it work like that at all 😅
How has this been tested?
Add a buttons block to your editor. Check that horizontal and vertical orientation, plus all the relevant justification options, still work as expected.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).