Buttons: Add variations for vertical layout #27297
Conversation
ntsekouras
added
[Block] Buttons
ntsekouras
requested review from
talldan,
ellatrix,
ZebulanStanphill and
nfmohit
|
Size Change: -808 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
ntsekouras
force-pushed
the
add/buttons-vertical-variation
branch
2 times, most recently
from
52e9602 to
60c4cae
Compare
|
Smoke tested on react-native-editor and there are no regressions there. If in the future we want to add this to mobile, it can be added. |
|
This is mostly working really nicely, though the spacing between blocks looks different in my local env to your screen capture: Not sure if this is a theme issue, it does look better in Twenty Nineteen and Twenty Twenty One Blocks, but the spacing is bigger in Twenty Twenty and Twenty Twenty One (non-blocks). The 'Use Theme Styles' toggle doesn't seem to be working anymore, so that's making it hard to check. |
There was a problem hiding this comment.
Technical implementation of the block looks good. It works similar to how Navigation block allows transforming its variations. save method was updated but it should produce the same output for existing content.
Let's make sure that the comment about CSS from @talldan is addressed before proceeding. @mapk or @jasmussen can you help with that?
|
It looks like integration tests for the Buttons block need to be regenerated to work with the new |
Hey @talldan - thanks for reviewing! This is something that can be affected by theme styles but your screenshot has big gaps :) . I tested with 2021(both blocks and non-blocks) and 2019 and seem quite good. In 2020 there are theme styles that cause that (margin top and bottom are 30px). Also previously the
What is this toggle? |
In the editor preferences Pretty useful for testing whether the default styling looks ok! I'll give it a re-test to see what's happening with the margins. |
ntsekouras
force-pushed
the
add/buttons-vertical-variation
branch
from
c37c86c to
f499ced
Compare
|
@ntsekouras It's this style in Twenty Twenty One non-blocks that's causing the gap in my local environment: I wonder if we're using different versions of the theme. Can you see this style if you inspect the button block wrapper? It might not be something to worry about in the core block styles, but perhaps we should make an issue over on the theme repo if we can validate the issue (I'm happy to do that): |
|
This is what I see..
Since is not merged I could ping some people who work on that theme to validate. I'll do that now. |
|
This is what I see:
My immediate instinct was that vertical or horizontal should just be style variations for the block. However as Nik kindly explained — the challenge with this block is the same as it is for the navigation block: style variations are just CSS changes, but these blocks need their horizontal movers swapped out with vertical movers, which require a different kind of transform. Because that is the case, this approach is okay. However, and not necessarily as part of this PR, but we really need to improve this selector:
It looks like a dropdown, but it doesn't act like a dropdown. It doesn't know its own transform, and it doesn't show it inside the dropdown field, as it should. Also because of that, when you click it the first time, there's no checkbox, which suggests you can transform a horizontal block to a horizontal block. It's not a good UI, and we really should improve it before we expand it that much further. Secondly, I wish there was a way to revisit the mover control so it could know itself whether to be vertical or horizontal, and avoid having us build these UIs on top of it. Could it simply read the flex-direction of its parent? |
|
@ntsekouras The screenshot looks like the styles for the buttons block. How about the styles for an individual button? |
I saw now that the margin applies on selecting the |
|
This is great, thank you! |
There was a problem hiding this comment.
Let's add an orientation prop to the <ContentJustificationDropdown> component, in order to provide a way to change the options shown to match the orientation. (Technically it would just rename them, I guess.)
In hindsight, when I implemented the content justification controls, I think I should have gone with start and end terminology for the variable/prop names rather than left and right, which don't make sense in a vertical context. Let's try and fix that while we're at it.
| @@ -18,10 +29,16 @@ | |||
|
|
|||
| &.is-content-justification-left { | |||
There was a problem hiding this comment.
Perhaps this className should have been is-content-justification-start, because "left" doesn't make sense in the vertical context, and the actual value of align-items that gets set is flex-start anyway. Same for is-content-justification-right which should probably be is-content-justification-end.
There was a problem hiding this comment.
Just realized the above comment was incorrect, since I got horizontal/vertical controls confused in my head.
Anyway, there is an actual problem here, though: this class is being used to align-items for vertical Buttons blocks, but the class name implies it can only set justify-content. Additionally, the justify-content styles are being applied to both vertical and horizontal orientations. That's not technically correct.
There was a problem hiding this comment.
Why not? You can have vertically stacked buttons and align them for example to the right. What do you propose exactly? If it's about the term/component names, do you think it's okay to have in a follow up PR?
There was a problem hiding this comment.
We shouldn't be setting justify-content for the vertical case... only align-items. But also, the class "is-content-justification-xxx" is kind of confusing if it's actually applying item alignment in the vertical cases. I think ideally, we'd want to tackle this stuff sooner rather than later to reduce the number of deprecations and legacy styles we have to keep on the front-end.
There was a problem hiding this comment.
The problem here is basically that I named the is-content-justification-xxx classes with the assumption that they would only be used to apply different justify-content styles, but that's no longer true in this case.
Maybe a new class name that can work both cases would be appropriate. I'm not sure what the correct term is, though.
Or maybe the solution is to apply different classes for the vertical cases, e.g. is-item-alignment-xxx. I'm not sure.
There was a problem hiding this comment.
personally I think of alignment as the positioning of the block itself, in this case the container block, and justificiation as the positioning of content inside.
It's worth remembering that in addition to align-items and justify-content, there's also align-content and justify-items, though justify-items doesn't do anything in Flexbox.
By looking at how these properties work, we can learn the following about how CSS is using these terms:
In a horizontally-flowing container, "align" refers to vertical positioning and "justify" refers to horizontal positioning. In a vertically-flowing container, "align" refers to horizontal positioning and "justify" refers to vertical positioning. In other words, "justify" refers to positioning along the primary axis that content is flowing, and "align" refers to the secondary (AKA "cross") axis.
"content" applies to all the children as a single block within the container, and "items" applies to each child as an individual unit.
Technically, the kind of alignment/justification we want for the horizontal variant is actually conceptually different from the control we want on the vertical variant. Technically, justify-content's cross-axis equivalent is align-content, not align-items. 😖
There was a problem hiding this comment.
If I had to come up with a better class name than what we're using right now, I guess something like is-content-horizontal-placement-xxx might work. I'm drawing the "placement" term here from the shorthand properties place-content and place-items. The CSS "placement" term is apparently its generic way of referring to both "alignment" and "justification".
Of course, in my suggested class name here, the meaning of "content" is slightly different from what CSS uses it for, since in the case of my suggested class name, it can refer to either CSS's "content" or "items". But I guess it's good enough for our purposes?
There was a problem hiding this comment.
For me it's too late to change the class names, this is already shipped and themes might be relying on it. The renaming doesn't seem to add a lot of value.
There was a problem hiding this comment.
As far as I can tell, it's only shipped in the plugin, not core, right? Does that change anything?
There was a problem hiding this comment.
Does that change anything?
It does, we have more luxury to change things that way but are we certain of that? It's also a bit late if it's in WP 5.6 as we're on RC2 and the stable is supposed to happen next week.
We need to do a few things if we want to rename the classnames:
- try to check usage of the classname in the plugins/theme repository
- backport the fix to WP 5.6 (so it needs to be in its own separate PR and not block this one)
|
|
|
@talldan Oh, right. Sorry, I got mixed up about what was going on in this PR. Taking a closer look, it's kind of weird that the component called |
ntsekouras
force-pushed
the
add/buttons-vertical-variation
branch
from
f499ced to
6d50534
Compare
|
Based on the above discussion about The next steps will be determined by:
|
Description
Resolves: #20081
This PR adds block variations to
Buttonsblock for enabling the switch toverticalandhorizontalorientation, in the same fashionNavigationblock handles this.How has this been tested?
Manually and by ensuring that previous versions of Buttons blocks would remain unaffected by this change.
Screenshots
Types of changes
Checklist: