-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dataviews list view: do not use Composite store #64987
Dataviews list view: do not use Composite store #64987
Conversation
2c97dca
to
8782bd0
Compare
function generateItemWrapperCompositeId( idPrefix: string ) { | ||
return `${ idPrefix }-item-wrapper`; | ||
} | ||
function generatePrimaryActionCompositeId( | ||
idPrefix: string, | ||
primaryActionId: string | ||
) { | ||
return `${ idPrefix }-primary-action-${ primaryActionId }`; | ||
} | ||
function generateDropdownTriggerCompositeId( idPrefix: string ) { | ||
return `${ idPrefix }-dropdown`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These three functions are used to generate an id
that gets associated with the different Composite.Item
components — the idea is that we explicitly assign those IDs, so that we can also use them to programmatically select a new active composite item when needed.
All composite Items share the same "prefix", which is determined by the data
item that the list receives.
return `${ idPrefix }-dropdown`; | ||
} | ||
|
||
function PrimaryActionGridCell< Item >( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to extract this component because the markup was already quite nested. Also, the new version makes it clear that there are two Composite.Item
that are exclusive to each other, based on the ( 'RenderModal' in primaryAction )
check.
There shouldn't be any changes to how this component works, apart from the explicit id
assignment to the Composite.Item
components, instead of passing the store
.
); | ||
} | ||
} } | ||
onKeyDown={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the logic to the parent component, where the rest of the active composite ID logic is.
const getItemDomId = useCallback( | ||
( item?: Item ) => | ||
item ? `${ baseId }-${ getItemId( item ) }` : undefined, | ||
const generateCompositeItemIdPrefix = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed getItemDomId
to generateCompositeItemIdPrefix
, as it better explains how composite items IDs are generated and assigned (also see https://github.com/WordPress/gutenberg/pull/64987/files#r1741698739).
I slightly changed the function's signature so that undefined
items are not accepted, which allows for simplifying the code below in a few places
const previousActiveItemIndex = usePrevious( activeItemIndex ); | ||
const isActiveIdInList = activeItemIndex !== -1; | ||
|
||
const selectCompositeItem = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function allows selecting + focusing a composite item, given its index
. It's the common code behind the two main bits of "controlled state" logic in this component:
- select a new active item if the current one gets removed (deleted);
- move the composite active selection up/down even when focussing a dropdown menu trigger button.
8782bd0
to
81ce43d
Compare
// Update the active composite item when the selected item changes. | ||
useEffect( () => { | ||
if ( selectedItem ) { | ||
setActiveCompositeId( | ||
generateItemWrapperCompositeId( | ||
generateCompositeItemIdPrefix( selectedItem ) | ||
) | ||
); | ||
} | ||
}, [ selectedItem, generateCompositeItemIdPrefix ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This replicates the same logic previously used for the defaultActiveId
prop in useCompositeStore()
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked well in my testing according to the test instructions 👍
Code-wise, I had a look on the surface and things looked well, but I'd prefer if @jorgefilipecosta or @oandregal could do a more thorough review to ensure we're not missing something.
4f3f816
to
0033f7a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks and tests well. I couldn't spot any regression. Thank you!
What?
Extracted from #64723
Refactor the dataviews list view component so that it doesn't use the
store
fromuseCompositeStore
Why?
See #63704 (comment)
How?
By controlling the
activeId
state via props and internal logicFollow-ups
[data-focus-visible]
instead of:focus-visible
and:focus-within
in order to follow ariakit's guidelines better.Testing Instructions (with related video captures)
Note: this PR shouldn't change how the app behaves compared to
trunk
.General navigation when loading the pages' screen
When focusing the widget for the first time, the active composite item should be the first one in the list — it should receive focus when tabbing on the composite widget.
dw-list-load.mp4
The active item matches the selected page on load
When loading the screen and a page is already selected (ie via the
postId
query parameter), the initial active composite item should be the one associated to the selected page. It should receive focus when tabbing on the composite widget for the first time.dw-list-load-with-selected.mp4
Using up/down arrows correctly moves the selection
While focus in on the composite widget, left/right arrow keys should move focus of the composite items in the same row. Up/down arrow keys should focus the equivalent composite item from the previous/next row.
This should still happen even when pressing the up/down arrow keys while focussing the dropdown menu trigger buttons. In this case, the dropdown menu should not open, and the composite selection should be updated instead.
dw-list-up-down-dropdownmenu-trigger.mp4
Deleting a page shouldn't cause the active item to get out of sync
If the page associated with the currently active composite item gets deleted, a new active composite item should be automatically picked — if possible, the item after the one that got deleted, otherwise the one before (in case the last page in the list was deleted).
dw-list-delete.mp4