Add new Parent block selector to child blocks (mahjong) #21056
Conversation
|
Got a quick first pass up that looks like this:
|
|
Size Change: +13 kB (1%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
|
Hey Shaun I really like example 2. With the selecting of the "Column" parent. |
|
This is cool. Do you have some ideas how user would be able to use the parent button using only keyboard navigation? |
|
@gziolo I imagine that you would get to it by shift-tabbing from the movers. In other words, you would shift-tab from the block icon to the movers, and then again to get to the select-parent button. That would make the most sense from a visual-order-should-match-DOM-order perspective. Right now, there is an inserter button when you shift-tab from the movers, but I think that's a poor place to put that control. See #13571. |
|
Here's the latest on this PR:
Things feel pretty good, though I could use some 👀 on the code — I'm certain there are missing things or better ways to accomplish some of these things.
You can put It's also worth noting that you can already use the keyboard's up/down arrows to move through the block hierarchy. |
|
I think that it would make more sense if the tab order was select-parent -> movers -> block switcher rather than movers -> select-parent -> block switcher. The select-parent button could be viewed as a sort of navigation link somewhat outside of the rest of the block toolbar (hence why it is floating a bit away from the rest of the toolbar). It's also the first thing in the visual order if you go top-bottom, left-right, so from an accessibility perspective, it should be the first thing in the DOM order. |
I agree that it makes sense to have the select parent button first. It's also a regular button so it can't be in the tab order in the middle of the toolbar. Keep in mind that we still didn't finish refactoring all tollbars to handle toolbar buttons navigation with arrow keys only and making the toolbar being a single tab stop. I guess for the accessibility reasons the parent selector should be always focusable even when it is not visible? I don't think it's implemented at the moment. |
|
Should we support multi-selection too? I believe it triggers an error now. |
|
Rocking it, Shaun! This is working better than I expected:
Two things:
I would disable it entirely for multi selection. Tab order feels correct to me, but I will defer to those with stronger opinions and background on best practices. |
Agreed. |
|
Pushed a handful of changes:
|
|
Just tested out this PR. This is fantastic! Already, I can see myself using this button far more often than the footer breadcrumb links. The only thing I'd change is the label (see code suggestion I recently made) and the DOM/tab order of the button. Aside from that, I think this is perfect. |
|
It works great, it's going to be awesome 💯 I second all the concerns @ZebulanStanphill shared. I recorder a screencast where I use the mix of tabbing and arrow keys (left/right) to navigate to parent blocks:
One thing I'm not sure about is whether this new "select parent" button should be part of the block toolbar and thus reachable with arrow left/right keys. It feels strange when you see how the focus moves at the moment because it's disconnected from the rest of the toolbar by both the spacing used and the direction that goes to the top. Maybe @enriquesanchez or @MarcoZehe could advise here on the best approach here. |
|
I took a test run using gutenberg.run. Before: It would be nicer to bring the popup "parent" icon closer to the toolbar. Here is a mockup. To give a feeling for how the parent icon looks when it is closer to the toolbar. It creates a closeness between objects. A stronger association. After: Bottom line: It will be helpful to bring "parent" icon a little closer to the toolbar and by doing so create an even stronger connection between the flow of the icons. |
|
I think the select-parent button should feel a little detached from the toolbar because it isn't part of the toolbar. I think that was the idea in the original mockup. It's sort of like the sibling inserter in that respect. The button could also be viewed as a sort of navigational skip link, similar to the one used to get to the Inspector. That's one of the reasons I think it should be the "leftmost" tabbable. Having it accessible in the middle of the toolbar doesn't make sense if it isn't part of the toolbar (both technically and visually. |
|
After the last rebase, I noticed that the tooltip for the Block Switcher component doesn't display when the block is within a parent block.
However, for blocks without a parent the tooltip appears to display normally:
And for the life of me, I can't figure out why... |
| * Block parent selector component, displaying the hierarchy of the | ||
| * current block selection as a single icon to "go up" a level. | ||
| * | ||
| * @return {WPElement} Parent block selector. |
There was a problem hiding this comment.
I'm pretty sure this should be WPComponent, not WPElement?
@shaunandrews Haii! I just took a looksy. I'm not too sure why either 🤔 I took a look at the My hunch tells me that there's probably a Edit: I checked out the custom hook that handled the mover interactions: Although it uses Will keep looking |
|
@shaunandrews I think I got it!
I pushed up my change. I followed the trail of props. It looked like the inner Let me know if that works for you! 🙏 |
|
@ItsJonQ Well that seems like a bug (outside of this PR); it seems like |
|
@ZebulanStanphill From what I can tell, it looks like cc'ing @diegohaz ! Not saying you broke anything 🙏 ! Just curious. It looks like the From a semantic perspective, Thanks! |
|
if it's a recent change (swapping label with title), then it's a breaking change and should be undone. Also consistency with the "Button" component seems like a good thing isn't it? |
As far as I know, the These props are named differently on <ToolbarButton title="title" isActive isDisabled />
<Button label="label" isPressed disabled />Besides that, to pass additional props like event handlers to the underlying <ToolbarButton extraProps={ { onKeyDown: ... } } />
<Button onKeyDown={ ... } />I'm not aware of the reasons behind that. But it seems to me that we should deprecate those different props on With #18931, |
@youknowriad Wonderful! I just pushed an update to the test snapshots. That should do it |
|
Thanks for that context @diegohaz !! |
|
Looks like Travis is happy! I'll merge it up. |
|
Really great to see this getting merged! I'm excited to see it in play. 🎉 |
|
Kudos @shaunandrews ! |
|
What a nice way to come back from a week offline — thanks for the assist, @ItsJonQ! |
This PR aims to create a UI that allows you to select the parent block from the currently selected block's toolbar: