List View: allow expanding and collapsing of nested blocks #32117
Conversation
|
Size Change: +1.46 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
Took this for a quick spin, even if it's still a work in progress. This is what I see:
Already now, this is an amazing improvement, it's incredibly better. I have a few design metrics that I'll help push so the various buttons are just right. This one also shows an ellipsis menu:
Even just in this state, it surfaces some interesting questions that I'm happy to chat through. For example: if you select a deeply nested block in the canvas, which is inside a collapsed group, does it uncollapse the group? And also, should the groups that you collapse stay collapsed even if you close and reopen the list view? Should every top level item start collapsed? All excellent questions that I imagine we can tune across a few PRs and not something we have to get right in this one. All to say, amazing work. |
|
Thanks for adding images @jasmussen, I ran out of time last week ✨
💯 I wanted to chat through this too! One thing that was much more jarring is that the primary action selects the block. This is not much of an issue with a mouse, but with a keyboard this means that focus is lost on the side panel, and a second enter creates a new paragraph, rather than toggling primary action.
There's a demo in treegrid on behavior https://www.w3.org/TR/wai-aria-practices/examples/treegrid/treegrid-1.html, where the first click selects, and the second click expands. I'm not sure I like the feel of the tristate toggle myself, but we could do something similar. demo.mp4Another thing that's different with the current implementation is that we're using a single td cell, and we need to be able to select nested items. This means that we need to reserve the primary action enter for selecting the block rather than expand/collapsing the item. Reserving Left and right keyboard events to expand and toggle feels okay.
I would expect it to retain remain collapsed, but also provide an expand all button/open all folders to the selected node. It's actually cheaper to not keep track of non-visible toggle state, so it's pretty simple to implement if we feel otherwise. We could also let other folks weigh in and modify this in follow up PRs.
It'd be much more performant for pages with lots of nested items to do this, but the tradeoff is more overhead for what is this/what do I do? I do think we'd want to hold off on trying this until we have implemented an open all nested items button, or open all items to our selected node target button.
To not let this get too out of scope, I'll aim to add the necessary missing keyboard events and try merging with behavior as is, unless folks spot anything that we should have for the first pass. |
|
Looking at some of the keyboard handling, it probably makes more sense to try and put the expand/collapse state + handling in the treegrid component, I'll give that a try next. |
Just to be sure, this is describing existing trunk behavior also, correct? It isn't specific to the addition of the expand/collapse chevron? Also CC: @Copons @jameskoster for some of the conversations around the list view. There may be prior art in the inserter, where if you tab to select a block and press Enter, it inserts it but doesn't move focus there. Whereas tab and press ⌘ + Enter, it inserts it and moves focus there. If I recall the conversation correctly, that is a known pattern for screen readers and interaction shortcuts.
👍 👍
Definitely good to keep this PR as small as possible, I only got excited about all the potential followups! And yes, I do think it'd be nice that a group you intentionally collapsed stays collapsed across your entire session, despite opening and closing the list view. |
I'm pretty sure this is the issue @Copons has surfaced a number of times: there's no streamlined way for one to move focus back to list view after selecting a block. The 'workaround' is that you need to close + reopen it which is one step more than it should be. One solution might be separate keyboard shortcuts for opening/closing rather than a single toggle, but that's probably something to explore separately. |
|
By the way, let me know when you're ready for me to push some polish here to the paddings (just little things) ✨ |
@jasmussen I'll give you a ping when I'm ready, I'm still sorting out the best place to handle state 👍 |
|
I added a quick work around for the keyboard support. @jasmussen feel free to add commits to the branch. 👍 |
| * WordPress dependencies | ||
| */ | ||
| import { chevronRight, Icon } from '@wordpress/icons'; | ||
| export default function BlockNavigationExpander( { onClick } ) { |
There was a problem hiding this comment.
Ideally in the future we don't need a component like this, and can instead use CSS rules to point at an SVG image.
|
I pushed a little polish:
This is based on the metrics in #32294 (comment), and involved creating a new "chevron-small" icon. I believe these utility icons are useful, just like we have a close and close small icon, for specific contexts like these. |
| @@ -121,8 +121,8 @@ | |||
|
|
|||
| .block-editor-block-icon { | |||
| align-self: flex-start; | |||
| margin-right: ( $grid-unit-05 * 2.5 ); // 10px is off the 4px grid, but required for visual alignment between block name and subsequent nested icon | |||
There was a problem hiding this comment.
CC: @jameskoster, the icon was scaled down to 20px, so it didn't show true pixels. I shifted the metrics around a bit, but would appreciate you taking a look when you have a chance, thank you.
|
One small issue I spotted: If you select and collapse a parent, it would be nice if the bottom border radii matched the top. Here's how it looks currently:
@jasmussen I appreciate the smaller chevron, but am wondering if it could be even smaller? I'm still feeling some tension between the block icon and the chevron. Here's a slightly downsized version (keeping the same overall footprint):
And an even more downsized version (1px stroke):
What do you think? |
jameskoster
added
Figma Component
|
I'd be on board with a smaller one, but I'd keep it 1.5px so as to not introduce new visual ingredients, which it'd feel like at 1px. |
|
It's a shame, the 1px feels better to me in practise, but I understand the reluctance to add more ingredients :D I pushed a smaller version of the 1.5px chevron to try. It's a little larger (filesize) because I outlined the stroke. |
jameskoster
removed
Figma Component
|
Nice one, thanks Jay! |
gwwar
force-pushed
the
try/collapsible-list-view
branch
from
14d82c8 to
c36de9e
Compare
|
This looks really slick! Hope to see it ship! |
| // Left: | ||
| // If a row is focused, and it is expanded, collapses the current row. | ||
| if ( activeRow?.ariaExpanded === 'true' ) { | ||
| onCollapseRow( activeRow?.dataset?.block ); |
There was a problem hiding this comment.
This and the similar line below couples the generic treegrid implementation to only work with elements that have a data-block attribute (blocks). I think it should ideally work for any kind of tree regardless of the content.
I'm not too precious about it (the whole thing could end up being deleted given the other discussions about moving away from tree grid in #32294 .)
But if there's a quick fix it would be good. Maybe pass the element back to the List View code or something, and that code can instead be responsible for accessing the dataset.block on the element.
There was a problem hiding this comment.
I think that's pretty fair. I've updated this in 90dd73f to pass back the row element instead
| return ( | ||
| // Keyboard events are handled by TreeGrid see: components/src/tree-grid/index.js | ||
| // eslint-disable-next-line jsx-a11y/click-events-have-key-events,jsx-a11y/no-static-element-interactions | ||
| <span |
There was a problem hiding this comment.
The implementation as a pseudo-element in the w3 example was really surprising to me.
I guess that makes it completely hidden from the accessibility tree, in which case the span could have aria-hidden="true" specified to replicate that.
It might be worth adding some comments to the component explaining why this is implemented like it is—I imagine many devs with good intentions might be tempted to change it to a Button which wouldn't be good given it's already rendered in a Button.
There was a problem hiding this comment.
Good call, I've updated this in f4b5a5f
For folks 👀 we can see the pseudo element in https://www.w3.org/TR/wai-aria-practices/examples/treegrid/treegrid-1.html
| @@ -17,6 +23,16 @@ import useBlockNavigationDropZone from './use-block-navigation-drop-zone'; | |||
| import { store as blockEditorStore } from '../../store'; | |||
|
|
|||
| const noop = () => {}; | |||
| const expanded = ( state, action ) => { | |||
There was a problem hiding this comment.
Yeah, agreed, it's probably not a big issue. I doubt this will ever become a huge amount of data.
I have ideas but I think it's an effort vs. reward question more than anything:
- BlockNavigation could receive stable block client ids from a selector (e.g. from
getBlockOrder), and reconcile it against theexpandedstate in an effect. - It could be possible to move the state to the treegrid component entirely. Could each
TreeGridRowmanage its own state? I think it'd then have to render its own expander component, but that seems potentially ok. I might be missing some complications.
| } | ||
| // If a row is focused, and it is expanded, focuses the first cell in the row. | ||
| getRowFocusables( activeRow )?.[ 0 ]?.focus(); | ||
| } |
There was a problem hiding this comment.
Looks like that's how a tree view works:
https://www.w3.org/TR/wai-aria-practices/#keyboard-interaction-22
So it might be determined by how #32294 is progressed. I'm not sure how that can be moved forwards, but probably worth coming to a decision soon.
Right we could try to simplify state by making this a boolean value on a child component versus a map in List View. Two things that stopped me from going with that approach were: Ideally, we do not render children in the DOM if it's collapsed. The List View mirrors the full document state in some use cases, which is a performance sensitive area. dom.mp4Persistence of toggle stage (Eg if we collapse child items, and then toggle a parent, do we retain state of those toggled child elements, or does it default to all toggled open?). retainstate.mp4 |
talldan
added
[Feature] List View
There was a problem hiding this comment.
Fantastic addition, great work @gwwar.
For a follow-up, I did notice that when closing and reopening list view, the expanded/collapsed state isn't retained, I guess a side-effect of the component no longer being rendered and the component state being lost.
|
Thanks for taking another look @talldan!
To support that we can move component state into a data store. Shouldn't be too bad to do, but we'll need to make sure that other list view instances are not sharing state. |
|
🥳 |
Another thing I wondered is whether this state should be part of the block editor store. Looking through that store, most of the actions/selectors relate directly to an actual block or block list, while this is a view on a block list, so it does feel slightly different to the main purpose of that store. 🤔 |
|
Awesome to see this, great work everyone! Really happy with all the iterations and refinements on this elements, it's becoming more and more useful and powerful. |
Part of #26141 to add the ability to expand and collapse parts of list view.
Updates in this PR include click support for expand/collapse and left and right keyboard events, as noted in https://www.w3.org/TR/wai-aria-practices/examples/treegrid/treegrid-1.html. A left keypress on a expanded item should collapse it, and a second keypress should navigate to its parent. Similarly a right keypress on a collapsed item should expand it.
I plan on splitting up work for List View expand/collapse in two parts:
Due to related interest, I also wrote up a quick issue around if Tree Grid still makes sense for this component #32294
expandcollapse.mp4
Requested Feedback 👀
The main thing that sticks out is state management and keyboard/focus handling. Right now keyboard handling is implemented at the Tree Grid level, which makes triggering callbacks at the branch level very difficult. I couldn't think of a good way of preserving nested collapsed state without resorting to a heavier data solution like a data store, or something a bit hacky like the click() call I made. Let me know if folks had thoughts around that.I think I have this mostly sorted with useReducer, but I can also make a data store here if folks would prefer that instead.Testing Instructions
nav.mp4