Add option for vertical orientation of Buttons block #20160
Conversation
nfmohit
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
|
Hey @nfmohit-wpmudev, thanks for picking this one up! One issue I spotted is that when the direction changes to vertical, the block mover buttons are still horizontal: I think this will be where using a style variation for the horizontal setting is not ideal, as it's difficult to detect when that option is active and then change the mover direction prop. I'll add the |
|
How could we handle alignment of the buttons? For example, if I want the buttons to be left, center, or right aligned? |
@shaunandrews The Buttons block has alignment options. That still seems to work fine in the PR. |
|
Hey @talldan ! 👋 Thank you very much for the review! I'm open to making any changes or adopting any different process for this here. Looking forward to the design review. Thank you ❤️ |
|
This looks really good! A very useful feature! Thank you Nahid for working on this feature! |
|
The behavior of the vertical Buttons style variant is just about perfect. The only issue I noted was the floating of the Buttons block container when aligned left or right. I'm unclear if that behavior would be expected by others. I expected the buttons to be text-aligned left or right instead of having the entire group floated. Here's a screenshot of the current behavior: Here's a screenshot of what I expected: |
|
@MichaelArestad The align-left/align-right actually being floats is a naming issue WordPress has had for years since the alignment tools for images were introduced in the classic editor. There's some discussion about it in #19672. |
Thanks for highlighting that. I was thinking, for this, we could change the controls from "Align" to the text alignment controls seen on a Paragraph block. |
nfmohit
force-pushed
the
add/vertical-style-buttons-block
branch
from
7188561 to
e45ebb6
Compare
|
Size Change: +3.84 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
Thank you for your review @MichaelArestad !
I completely agree with you there @talldan. I've tried but couldn't find a simple way to get the style as a prop. There are custom hacks, but I'm sure they're not ideal to use. Do you have any recommendations on our way forward here? |
|
@nfmohit-wpmudev I think potentially using a style variation might not be the best way to go for this. Maybe a simple RadioControl (https://github.com/WordPress/gutenberg/tree/master/packages/components/src/radio-control#design-guidelines) that sets an attribute could be an option. That attribute could then be used to set the mover direction and a class name. It could also be a toolbar option (a bit like Media and Text has for Show Media on Right / Left), but it'd need a good icon. Will add |
|
I added a link to the "Navigation Block: add support for vertical menus" above. Both are looking for a good way to add verticality. |
It's worth noting that in that case, the option is an attribute because it actually changes the HTML markup, so that the DOM order continues to match the visual order. In the case of the Buttons block, a vertical style doesn't change the visual order of the buttons. I'd be wary of adding block attributes for something that can be accomplished through styling alone. |
@ZebulanStanphill In this case a value is needed for the |
|
Thank you very much for your inputs everyone. I'm looking forward to the design feedback and then will be happy to move ahead with the suggested solution. |
Then I think the way the mover direction is handled needs to be tweaked. It would make more sense if the mover direction could be changed based on the block style, by specifying the desired orientation when you register a style variation. That way, themes (and eventually also users) could add their own block styles that change horizontal things to vertical, and change the mover orientation as needed. |
I agree with @ZebulanStanphill here. I feel the best way to visually present a style like this is definitely using block styles. In this case, the change should be considered in either the mover, or ideally in the Block Styles so that some kind of a value can be extracted along with the addition of a CSS class. |
There was a problem hiding this comment.
LGTM from a code point of view, and worked well when I tested.
Regarding that remaining alignment issue, maybe move that to a separate issue/PR so that it's captured after this one's merged.
I still think it'd be great to get some feedback from a design point of view. Maybe you could drop a note in the #design slack channel @nfmohit-wpmudev?
I'd like to think any of those changes could be worked on iteratively though, and what's currently in this PR is a good first step.
Edit: I renamed the PR so that it's more descriptive in the changelog.
talldan
changed the title
|
Taking a look at gutenberg.run. Back on track. Hit zone around each individual button in vertical mode is larger compared to the hit zone for horizontal mode. Making it harder to click just outside the buttons to select the parent Buttons block. There seems to be a grey zone buffer area. Clicking into it might select a button or the parent Buttons.
Regarding alignment. For instance with alignment (none) Then switching to alignment right. Is lower then alignment none. An inserter suddenly showed up to the right of the vertical menu. As I was clicking randomly to select the parent Buttons block. EDIT: |
|
Again, I don't think the Buttons block should even have the center/left/right "alignment" options. They don't make sense for the block. The block should have item alignment options like the Navigation block. |
|
Thank you for your observations @paaljoachim ! @talldan When you have a minute, could you possibly take a look at the points @paaljoachim mentioned and point me towards a direction? The discoverability issue is something I have noticed myself too. Is that something we should address in this PR? For the alignment and the spacing issue, I think that also exists for the horizontal orientation and is not only specific for the vertical orientation. Ideas? I'm not able to replicate the issue with that random inserter. I'm a bit confused with the way to go forward. I'm also considering @ZebulanStanphill's feedback here and thinking about revamping the alignment options in a new PR. Once we address the above concerns, I'll surely go ahead and request feedback from the #design channel. I'll be grateful for any suggestion. Thank you guys! |
|
@nfmohit-wpmudev Looks to me like the bug @paaljoachim mentioned is also in Efforts to fix it separately would be greatly appreciated. @paaljoachim would you be able to make an issue? I don't think there are any other blockers, other issues could be tackled separately. But I do still think it'd be good to get a design thumbs up on this as a first draft, there wasn't much input from design throughout developing this feature. Eventually I think this option could become a Block Variation, but there are a few issues that need to be tackled before that:
Possibly also: |
|
Hey @talldan First off I believe there should be a common approach and UI method for the showing of vertical orientation for the Navigation block, Social Icons block and the Buttons block. Styles for the Buttons block work really well, as a style is something that affects the look of the block. The same method should then also be applied to other related blocks. Are you thinking of the bug I mentioned where the vertical Buttons Block jumps higher up vertically on the page? |
|
Hey @paaljoachim Yep, I think the common approach would be to use Block Variations, which the navigation block started doing. The navigation block is experimental though, those two issues I mentioned need to be solved before this can really be tried on a non-experimental block.
Yep, that's what I was referring to. 👍 |
|
I wonder if we can temporarily add the vertical style for the Buttons Block. In the following comment I am suggesting a new panel named Layout to be used for layout variations. #20584 (comment) Here is a new issue for the jumping up and down of the vertical Buttons Block. |
|
I think there likely needs to be some followup PRs to this. I'm going to list a few of the bugs I noticed:
I would like to see these all resolved if possible before I would recommend merging this PR. As far as design, I'm not sure the sidebar is the right place. Perhaps block variations (like the nav block) would make more sense. For now, I'm okay with it in the sidebar, but I think the section label needs to change from "Button Settings" to "Layout." |
|
@MichaelArestad It's not a bug for only the container to be aligned by the block alignment options. Rather, it's probably a bug that the Buttons block even has center/left/right block alignment options in the first place. It should have item alignment/justification options instead. Perhaps a new PR should be opened to resolve this issue first? |
|
Thank you for your feedback @MichaelArestad ! I second what @ZebulanStanphill's said. What do you think @talldan @MichaelArestad ? |
|
A quick mockup comparing the two. Buttons settings. Layout |
@ZebulanStanphill @nfmohit-wpmudev It took me a minute to understand the distinction, but that makes sense. I'd like to see the current control removed in the meantime as I think it just adds confusion rather than working as expected. |
|
I began work on #23168 to overhaul the alignment/justification controls. However, I realized that to get the implementation working better, I should first update the block's editor markup to match the front-end more closely. So I created #23222 to do that, but now that PR is stuck due to technical complications with the unique way the Buttons block disables alignment controls for nested Button blocks. I have a hunch that the changes in #23034 might help with all this, but that PR is also in need of technical help at the moment. So unfortunately, I think it's going to take some time before this PR can get merged, if we want to make sure the existing features on the Buttons block are polished and working before adding any more. 😞 |
|
#23034 is merged, does it unblock this one? |
|
@youknowriad Technically, #23168 is the PR that is blocking this one. And speaking of which, that PR should be ready for an initial review now, since #23222 was merged. |
|
I'm unable to get this PR running, but I believe (as mentioned above) the horizontal/vertical option should be a Variation of the button block rather than a Style. The Buttons block already contains two Styles; Fill and Outline.
If we add horizontal and vertical Styles, I think we'd have to actually add more styles to account for the combination of Fill, Outline, Horizontal, and Vertical. (i.e. Horizontal Filled, Horizontal Outline, etc, etc) |
|
I tested Social Icons block to compare how things are done there. Horizontal/vertical options should not be added to the Styles panel. We should create a common phrase/term for these options. Perhaps (as I mentioned above) "Layout" or "Structure". Layout could perhaps also be used for other blocks where one adjusts the layout/structure of a block. We need a new term for structural adjustments. For horizontal/vertical structure. For Column block structure. For Media & Text structure. Etc. |
|
@shaunandrews Unfortunately the screenshot in the description is outdated, I've removed it! Currently it's implemented as a radio button like @paaljoachim's comment here - #20160 (comment) This can be migrated to a block variation in the future, but it hasn't been implemented that way right now as I don't think there's a way to switch between variations easily (which feels like something that will be needed). |
|
#23168 is merged now, so this can be rebased to work with the new content justification controls. |
|
What is needed to move this PR onward? Thanks! |
|
I believe this PR could use the same treatment as the vertical/horizontal variation that was added to the Navigation block. |
Yeah, I think it's a very similar use case and could work well with the variations. We don't need to expose them to the inserter though. We could just set as |
Description
This PR tries to close #20081 which requests the addition of a vertical style to the buttons block.
How has this been tested?
This PR has been tested by going through the following steps:
Screenshots
Types of changes
Non-breaking enhancement
Checklist: