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.

Sign up for GitHub

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

Jump to bottom

Dataviews list view: do not use Composite store #64987

Merged

ciampo merged 10 commits into trunk from feat/stabilize-composite/dataviews-list-view-no-composite-store

Sep 3, 2024

Contributor

ciampo commented Sep 2, 2024 •

edited

Loading

What?

Extracted from #64723

Refactor the dataviews list view component so that it doesn't use the store from useCompositeStore

Why?

See #63704 (comment)

How?

By controlling the activeId state via props and internal logic

Follow-ups

Look into the styles, and see if we can use [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

ciampo mentioned this pull request

Composite: remove store prop and useCompositeStore hook #64723

Merged

ciampo self-assigned this

ciampo added [Type] Enhancement [Package] DataViews labels

ciampo force-pushed the feat/stabilize-composite/dataviews-list-view-no-composite-store branch 2 times, most recently from 2c97dca to 8782bd0 Compare

September 3, 2024 08:57

ciampo commented

View reviewed changes

packages/dataviews/src/dataviews-layouts/list/index.tsx

Comment on lines +58 to +69

+              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`;
+              }

Contributor Author

ciampo Sep 3, 2024 •

edited

Loading

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.

ciampo commented

View reviewed changes

packages/dataviews/src/dataviews-layouts/list/index.tsx

+              	return `${ idPrefix }-dropdown`;
+              }
+              function PrimaryActionGridCell< Item >( {

Contributor Author

ciampo Sep 3, 2024 •

edited

Loading

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.

ciampo commented

View reviewed changes

packages/dataviews/src/dataviews-layouts/list/index.tsx Show resolved Hide resolved

ciampo commented

View reviewed changes

packages/dataviews/src/dataviews-layouts/list/index.tsx

-              														);
-              													}
-              												} }
+              												onKeyDown={

Contributor Author

ciampo Sep 3, 2024

Moved the logic to the parent component, where the rest of the active composite ID logic is.

ciampo commented

View reviewed changes

packages/dataviews/src/dataviews-layouts/list/index.tsx

-              	const getItemDomId = useCallback(
-              		( item?: Item ) =>
-              			item ? `${ baseId }-${ getItemId( item ) }` : undefined,
+              	const generateCompositeItemIdPrefix = useCallback(

Contributor Author

ciampo Sep 3, 2024

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

ciampo commented

View reviewed changes

packages/dataviews/src/dataviews-layouts/list/index.tsx

+              	const previousActiveItemIndex = usePrevious( activeItemIndex );
+              	const isActiveIdInList = activeItemIndex !== -1;
+              	const selectCompositeItem = useCallback(

Contributor Author

ciampo Sep 3, 2024

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.

ciampo force-pushed the feat/stabilize-composite/dataviews-list-view-no-composite-store branch from 8782bd0 to 81ce43d Compare

September 3, 2024 09:19

ciampo commented

View reviewed changes

packages/dataviews/src/dataviews-layouts/list/index.tsx

Comment on lines +373 to +382

+              	// Update the active composite item when the selected item changes.
+              	useEffect( () => {
+              		if ( selectedItem ) {
+              			setActiveCompositeId(
+              				generateItemWrapperCompositeId(
+              					generateCompositeItemIdPrefix( selectedItem )
+              				)
+              			);
+              		}
+              	}, [ selectedItem, generateCompositeItemIdPrefix ] );

Contributor Author

ciampo Sep 3, 2024

This replicates the same logic previously used for the defaultActiveId prop in useCompositeStore()

ciampo marked this pull request as ready for review

September 3, 2024 10:00

github-actions bot commented Sep 3, 2024 •

edited

Loading

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ciampo requested review from oandregal, jorgefilipecosta, ntsekouras and a team

September 3, 2024 10:00

tyxla reviewed

View reviewed changes

Member

tyxla left a comment

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.

ciampo added 9 commits

September 3, 2024 17:04


          Move over changes from POW PR

adbde7a


          Simpler new active item comment


          Simplify dropdown trigger keydown override

96f8e2c


          Simplify getItemDomId

5e6dd96


          Further simplify dropdown menu trigger keydown handler

e93accb


          Extract primary action grid cell to separate component

2a23d34


          Sort render function order for better logic flow

1e2a2ff


          Extract more logic to generate IDs + always use ID prefix

b3472e5


          Fix buggy initial active composite ID logic

9a55a04


          Add key to primary action gridcell

0033f7a

ciampo force-pushed the feat/stabilize-composite/dataviews-list-view-no-composite-store branch from 4f3f816 to 0033f7a Compare

September 3, 2024 15:06

ntsekouras approved these changes

View reviewed changes

Contributor

ntsekouras left a comment

This looks and tests well. I couldn't spot any regression. Thank you!

ciampo merged commit 14a8cf3 into trunk

62 of 63 checks passed

ciampo deleted the feat/stabilize-composite/dataviews-list-view-no-composite-store branch

September 3, 2024 16:10

github-actions bot added this to the Gutenberg 19.2 milestone

bph added the [Feature] Data Views label

WontonSam mentioned this pull request

[Snyk] Upgrade @wordpress/eslint-plugin from 11.1.0 to 20.3.0 WontonSam/woocommerce-admin#266

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Data Views [Package] DataViews [Type] Enhancement