DataViews: add actions to list layout #60805
Conversation
oandregal
added
[Type] Enhancement
|
Size Change: +164 B (+0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
#59637 introduced arrow-key navigation for the list items. I don't remember the details, but my expectation was that there was a single tab stop for the list items (the whole item) and the actions button (called details button at the time) was reachable via arrow keys (right/left). Code changed a bit since and this PR re-introduces the actions button, though it's not working as I expected (note how the tab navigation actually goes into the action button and right-left arrow keys don't work): Gravacao.do.ecra.2024-04-17.as.11.37.30.movCompare with the editor's list view behaviour: Gravacao.do.ecra.2024-04-17.as.11.43.53.mov |
oandregal
force-pushed
the
add/actions-to-list-layout
branch
from
684cd5d
to
2f42280
Compare
|
By adding a second
According to the AriaKit's docs for Composite, two-dimensional navigation works by using the following (we use a store instead of a CompositeProvider): <Composite store={store}>
<CompositeRow>
<CompositeItem>Item 1.1</CompositeItem>
<CompositeItem>Item 1.2</CompositeItem>
</CompositeRow>
<CompositeRow>
<CompositeItem>Item 2.1</CompositeItem>
<CompositeItem>Item 2.2</CompositeItem>
</CompositeRow>
</Composite>That's our basic structure, though we use the render prop to control what's rendered for the CompositeItem: <Composite store={store}>
<CompositeRow>
<CompositeItem>Item 1.1</CompositeItem>
<CompositeItem render={<button>Item 1.2</button>} />
</CompositeRow>
<CompositeRow>
<CompositeItem>Item 2.1</CompositeItem>
<CompositeItem render={<button>Item 2.2</button>} />
</CompositeRow>
</Composite>This is all well, and works as expected (codesandbox demo): Gravacao.do.ecra.2024-04-30.as.14.10.23.movHowever, this stops working as soon as the Pinging some of the @WordPress/gutenberg-components minds for feedback. |
|
Hi there @oandregal 👋 😄 I want to help. Two questions:
cc @mirka as I believe you were looking into this. I could take a look and try to fix this in Ariakit's side. |
|
Hi @DaniGuardiola thanks for offering help :) Just this morning was tinkering with the example Lena shared in slack using only Ariaki components, and modified it so it has multiple rows. It demonstrates how Menus don't work well with composite (or there's some config that I'm missing). I've looked into Ariakit's Github, and didn't find any issue for this. The only issue related to composite is ariakit/ariakit#3170 created by Andrew, but it's a bit different one. Issue created upstream at ariakit/ariakit#3768 |
| actions, | ||
| data, | ||
| fields, |
There was a problem hiding this comment.
This is just sorting the props alphabetically.
| @@ -175,14 +175,14 @@ function GridItem( { | |||
| } | |||
|
|
|||
| export default function ViewGrid( { | |||
| actions, | |||
There was a problem hiding this comment.
This is just sorting the prop alphabetically.
| actions={ actions } | ||
| data={ data } | ||
| fields={ _fields } |
There was a problem hiding this comment.
Sorts the props alphabetically.
oandregal
force-pushed
the
add/actions-to-list-layout
branch
from
2f42280
to
f377520
Compare
|
As per the conversation at ariakit/ariakit#3768 and this example by Dani https://stackblitz.com/edit/vitejs-vite-p27qhy?file=src%2FApp.tsx&terminal=dev there seems to be a way to make two dimensional arrow key navigation work with Ariakit's components. Specifically, this bit is the key: <Ariakit.CompositeRow>
<Ariakit.CompositeItem render={<button>Button</button>} />
<Ariakit.MenuProvider>
{/* here, CompositeItem has precedence and pressing arrow down
goes downwards in the 2d composite as expected */}
<Ariakit.CompositeItem
store={store}
render={<Ariakit.MenuButton>Menu</Ariakit.MenuButton>}
/>
<Ariakit.Menu>
<Ariakit.MenuItem>Hello</Ariakit.MenuItem>
</Ariakit.Menu>
</Ariakit.MenuProvider>
<Ariakit.CompositeItem render={<button>Button</button>} />
</Ariakit.CompositeRow>Gravacao.do.ecra.2024-05-08.as.14.11.31.movI'm looking into how we can achieve the same with the existing |
|
Sharing my ongoing work, as per the slack conversation. In dc0f82d I switched to use the underlying Ariakit components directly, and it's working: Gravacao.do.ecra.2024-05-08.as.16.26.14.movOf course, we want to use the |
| onSelectionChange, | ||
| selection, | ||
| id: preferredId, |
There was a problem hiding this comment.
Removing it because it's not used.
|
Using the <Ariakit.MenuProvider>
<CompositeItem
store={ store }
render={
<Ariakit.MenuButton>Menu</Ariakit.MenuButton>
}
/>
<Ariakit.Menu>
<Ariakit.MenuItem>Hello</Ariakit.MenuItem>
</Ariakit.Menu>
</Ariakit.MenuProvider> |
|
I'm not sure why we want to use wordpress components here. Are the styles the exact same? These components are little more than themed and slightly more abstracted Ariakit components, not necessarily meant to be low-level. I think using Ariakit components here directly is fine unless you need to reuse a significant mount of styles from the wordpress component in question. Edit: I think I misunderstood what's being done here. Is this DataViews component supposed to be composed with other components by its consumer? |
|
As of f10e368 I've got it working with Gravacao.do.ecra.2024-05-08.as.17.18.50.movIt's not complete, I need to add support for primary actions, verify everything is working properly, etc. Also see if there's ways to modify |
As far as my understanding goes, using Ariakit directly is not allowed — there's even lint rules that prevent you from committing if you do so. I'm not sure why this is. Anyway, I've got this PR working with |
This is correct and very intentional. The |
|
Interaction-wise, this is working now with every kind of action (modal & non modal): Gravacao.do.ecra.2024-05-09.as.11.11.09.movI still have to test all action flows and verify that everything works properly. I'm also looking for opportunities to improve the existing code now that it's working, so things can change a bit code-wise. @youknowriad @jameskoster While I do that, I'd welcome feedback on the following:
The original issue suggested only Edit #59659 and that sounds good to me, but I wanted to double check. I plan to implement this in consumer-land, not as a new dataviews API: this is, the Pages component will change the primary actions depending on layout. |
| @@ -401,6 +401,7 @@ | |||
| li { | |||
| margin: 0; | |||
| cursor: pointer; | |||
| border-top: 1px solid $gray-100; | |||
There was a problem hiding this comment.
Move the border to the topmost HTML element. The .dataviews-view-list__item only targets the item but not the actions.
Edit as the only primary action makes sense but I actually wonder about adding more things to think about from a consumer's perspective. What if we just say: The list action can only have one primary action and it's always the first action? |
packages/dataviews/src/view-list.tsx
Outdated
| primaryField?: NormalizedField; | ||
| store: any; |
There was a problem hiding this comment.
Unsure if there's a better way to type this https://ariakit.org/reference/use-composite-store
There was a problem hiding this comment.
There was a problem hiding this comment.
I've been trying to avoid us having to export types from the package, to reduce maintenance cost. The suggested way is to extract the types from the components/hooks that we export.
type CompositeStore = React.ComponentProps< typeof Composite >[ 'store' ];
type CompositeStoreProps = Parameters< typeof useCompositeStore >[ 0 ];Though, I was looking at this specific case and noticed that the unlock() function for private APIs is typed in a way that doesn't preserve the original types of the locked objects. I'll look into that.
I wouldn't be opposed to that, but I also don't mind keeping all primary actions visible for consistency with table layout, so long as we tweak the visuals a little: 
|
oandregal
force-pushed
the
add/actions-to-list-layout
branch
from
1a5b41d
to
b003ae4
Compare
| interface ListViewProps { | ||
| view: ViewListType; | ||
| fields: NormalizedField[]; | ||
| actions: Action[]; |
There was a problem hiding this comment.
This is the only new prop, the other changes sort the props alphabetically.
|
|
||
| interface Action { | ||
| callback: ( items: Item[] ) => void; | ||
| icon: any; |
There was a problem hiding this comment.
I'm not sure how to provide the type for this. Happy to prepare a follow-up to improve it.
There was a problem hiding this comment.
I've actually provided types for all the actions in #61400 but unfortunately my PR is blocked because of some unrelated failure. Maybe we should just extract the action types from my PR and land it (without actually typing item-actions.js). I'll try this in its own PR and we can reuse these types here instead.
There was a problem hiding this comment.
I actually tried adding IconType as you do in your PR, but that made this PR fail locally.
| @@ -192,6 +369,24 @@ export default function ViewList( props: ListViewProps ) { | |||
| defaultActiveId: getItemDomId( selectedItem ), | |||
| } ); | |||
|
|
|||
| // Manage focused item, when the active one is removed from the list. | |||
| const isActiveIdInList = store.useState( | |||
| ( state: { items: any[]; activeId: any } ) => | |||
There was a problem hiding this comment.
I'm not sure how to provide the type for this. Happy to prepare a follow-up to improve it.
There was a problem hiding this comment.
Not sure exactly either, I guess this needs usage of ariakit types?
Regardless, it's a bit weird that we have to use all these low level hooks (store and what not). I'm guessing the components API of ariakit is not easily usable for us here?
There was a problem hiding this comment.
Regardless, it's a bit weird that we have to use all these low level hooks (store and what not). I'm guessing the components API of ariakit is not easily usable for us here?
Yeah. Each has its own reason:
- As per Composite: two-dimensional arrow navigation with MenuButtons ariakit/ariakit#3768 the store is a requisite for each
CompositeItem - As per DataViews: add actions to list layout #60805 (comment) we need to handle onkeydown for the "all actions" button to make sure arrow down doesn't open the menu but goes down the list. This is an issue with our Dropdown, as it works fine with Ariakit components.
- As per DataViews: add actions to list layout #60805 (comment) when deleting an element, we need to take care of updating
activeIdas ariakit doesn't do it for us.
There was a problem hiding this comment.
Thanks for the details.
we need to handle onkeydown for the "all actions" button to make sure arrow down doesn't open the menu but goes down the list. This is an issue with our Dropdown, as it works fine with Ariakit components.
Maybe I'm not testing properly but for me it was actually opening the menu (and I felt that it was ok :P )
As per #60805 (comment) when deleting an element, we need to take care of updating activeId as ariakit doesn't do it for us.
Couldn't that be considered a bug to be solved in ariakit?
There was a problem hiding this comment.
Just saw the ariakit issue :)
There was a problem hiding this comment.
cc @WordPress/gutenberg-components for awareness. This PR implements two-dimensional arrow-key navigation with Ariakit's Composite & our own components. I'm leaving some comments for your awareness, in case we want to smooth it as an API. I plan to do similar work for other dataviews' layout in a few weeks.
I had to use the store's state in this PR, so I needed access to some types. I presume any can be improved by using Ariakit's types directly, but I also want to bring it up as another case where we may want to re-export from our components package.
|
All issues and feedback have been addressed, just waiting for an approval :) |
|
In terms of behavior this is working great for me. I did notice the double snackbars (inside and outside the preview but these should probably be solved separately) |
| @@ -142,23 +205,137 @@ function ListItem( { | |||
| </HStack> | |||
| </CompositeItem> | |||
| </div> | |||
| { actions?.length > 0 && ( | |||
There was a problem hiding this comment.
I don't understand why we can't reuse item-actions here? (Maybe with a new prop that indicates that we should only use one primary action). What's the other differences between this and item actions?
There was a problem hiding this comment.
These are the differences: every button trigger needs to be wrapped by a CompositeItem that receives the composite store, the "all actions" button needs to handle onkeydown event on their own, it only renders 1 primary action.
An alternative to make ItemActions work for this layout would be adding ifs everywhere, though I discarded it because it is too intertwined. When table and grid layout improve the keyboard interactions, ItemActions would actually be more reusable as all the layouts would share the same logic.
There was a problem hiding this comment.
When table and grid layout improve the keyboard interactions, ItemActions would actually be more reusable as all the layouts would share the same logic.
With the gained ariakit/composite experience, I actually want to tackle that next, though it'll probably land for 6.7.
There was a problem hiding this comment.
An alternative to make ItemActions work for this layout would be adding ifs everywhere, though I discarded it because it is too intertwined. When table and grid layout improve the keyboard interactions, ItemActions would actually be more reusable as all the layouts would share the same logic.
How can we make sure we actually do that and not end up with two similar components.
There was a problem hiding this comment.
How can we make sure we actually do that and not end up with two similar components.
We'll have more info about the proper abstractions for the actions component when keyboard interactions are improved for other layouts. The existing proved not enough to use with Composite, for example. Personally, I won't be able to start looking into this until end of May/first of June, the soonest. If we'd rather have a single component that accommodates all the possible use cases in the interim, I'm happy to switch the implementation.
| store.move( store.up() ); | ||
| } | ||
| } | ||
| }, [ isActiveIdInList ] ); |
There was a problem hiding this comment.
This is a very weird effect, why do we have this adhoc logic in our view?
There was a problem hiding this comment.
It fixes this behavior (see related ariakit issue).
There was a problem hiding this comment.
cc @WordPress/gutenberg-components for awareness. This PR implements two-dimensional arrow-key navigation with Ariakit's Composite & our own components. I'm leaving some comments for your awareness, in case we want to smooth it as an API. I plan to do similar work for other dataviews' layout in a few weeks.
This makes sure the internal ariakit index is updated when an element is updated (see).
|
I'm adding e2e tests for this at #61648 |
|
Nice work on this 🚀 A couple of follow-ups;
|
| onMouseEnter={ handleMouseEnter } | ||
| onMouseLeave={ handleMouseLeave } |
There was a problem hiding this comment.
Super minor, but with these tiny functions I think it's more readable if it's all inlined (especially since they aren't even using useCallback:
onMouseEnter={ () => setIsHovered( true ) }| onActionStart={ () => {} } | ||
| onActionPerformed={ () => {} } |
There was a problem hiding this comment.
Why pass empty functions at all?
There was a problem hiding this comment.
Typescript :)
Thanks for bring it up. I added this at some point so Typescript let me focus on behavioral issues rather than ortographical things but forgot to add a TODO marker to come back to it later. Prepared a follow-up to fix this at #61659
| @@ -2,6 +2,9 @@ | |||
| * External dependencies | |||
| */ | |||
| import clsx from 'clsx'; | |||
| // Import CompositeStore type, which is not exported from @wordpress/components. | |||
| // eslint-disable-next-line no-restricted-imports | |||
| import type { CompositeStore } from '@ariakit/react'; | |||
There was a problem hiding this comment.
cc @WordPress/gutenberg-components for awareness. This PR implements two-dimensional arrow-key navigation with Ariakit's Composite & our own components. I'm leaving some comments for your awareness, in case we want to smooth it as an API. I plan to do similar work for other dataviews' layout in a few weeks.
I presume we should re-export the type from our components package?
| icon={ moreVertical } | ||
| label={ __( 'Actions' ) } | ||
| disabled={ ! actions.length } | ||
| onKeyDown={ ( event: { |
There was a problem hiding this comment.
cc @WordPress/gutenberg-components for awareness. This PR implements two-dimensional arrow-key navigation with Ariakit's Composite & our own components. I'm leaving some comments for your awareness, in case we want to smooth it as an API. I plan to do similar work for other dataviews' layout in a few weeks.
I had to implement the onKeyDown event in the trigger Button to prevent the arrow down from opening the menu. That's not necessarily when using Ariakit directly, see example at ariakit/ariakit#3768
|
Even though the "navigate" metric in the site editor is a bit unstable in our tests, I do think this PR had a small impact on the numbers there. I wonder if you have ideas. |
Yeah, I agree. I've tried before/after locally and I see a consistent +15ms after this PR. This is what I've looked at (the same as the navigate performance test):
Codehealth after this commit reports a higher mean number as well: 
I've looked at browser's performance traces and they are very similar (before.json.zip & after.json.zip, for anyone to import into devtools and inspect):
The major thing I've noticed is the Role component coming from Ariakit's reac-core package (imported from
So I guess it's fair to say that the performance hit comes from the internal components/ariakit code, which is in line with what this PR does:
Further debugging would require looking more deeply at how |
|
I suspect it's the fact that we're actually rendering more components (The actions dropdown) and re-rendering them when we select new pages. One thing to check is whether these things re-render too much and if we can limit the re-renderings. |
Part of #59659 and #55083
Follow-up to #59637
Fixes #61555
What?
This PR adds support for actions in list layout, which is only enabled for Pages and Templates as of now:
This PR also enables two-dimensional arrow-key navigation:
Gravacao.do.ecra.2024-05-10.as.11.53.21.mov
Why?
We want to make them available. See #59659
How?
actionsprop that any other layout is using to the list view layout.Testing Instructions