DataViews: simplify selection setting #62846

ellatrix · 2024-06-25T15:29:54Z

What?

Changes the internal onSelectionChange function to simply be the React state set function, and set selection via IDs rather than items.
This allows us to internally set selection with a callback.
Simplify some of the internal state setting to use a callback function.
Fix types.

Why?

Simplicity.

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions · 2024-06-25T15:38:42Z

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: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: oandregal <oandregal@git.wordpress.org>

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

youknowriad · 2024-06-25T15:42:50Z

packages/dataviews/src/types.ts

-import type { ReactElement, ReactNode } from 'react';
+import type { ReactElement, ReactNode, Dispatch, SetStateAction } from 'react';
+
+export type setSelection = Dispatch< SetStateAction< string[] > >;


What does "dispatch" do?

--
The rest of the comment here is not blocking, mostly discussion and typescript learnings / guidelines discussion.

I think there's a balance to be found between the types that make sense as exported or not. For smaller types like this, I think duplication may be fine (avoid indirection). But I don't know if folks more familiar with typescript think. cc @sirreal @jsnajdr

Also curious what @mcsf thinks here.

I can revert 2e2f38c if you like

Oh I see what Dispatch does, it's useful I'd say :)

Curious about the guidelines on exposing this kind of low level types.

Yeah, I forgot I changed a public type too. I can revert it to ( selection: string[] | ( ( prevState: string[] ) => string[] ) ) => void to unblock if you prefer.

My feedback is that it just took me non-trivial effort to figure out what this Dispatch<SetStateAction> actually is. Better to write it plainly as @ellatrix proposes. Also make the first letter of the type name uppercase, SetSelection 🙂

Convention is typically to capitalize types, it might be good to change the type name setSelection to SetSelection.

This type seems to be used repeatedly and could be error prone to write out over and over, it seems like there's value in a reusable type.

As far as exporting the type or not, I'm not sure how important the questions is. The truth is that any types that are part of the public interface are effectively public types because they will be visible externally.

This type is directly exported, so I guess that may be the question:

gutenberg/packages/dataviews/src/index.ts

Line 4 in 1ee2e3c

export type * from './types';

There, I would tend to be cautious. Is this something that is immediately useful for third parties? If not, then we might still export a single type and use it throughout the repository, but not export it directly. Again, ultimately the type will be visible and therefore accessible externally, the question is really about how much of the public API or how visible it should be. It's not the same to access it in laborious ways or just import the type.

It looks like this type is changing, but it may be strictly wider? All values that satisfied the old type also satisfy the new type? In that case I don't think it would qualify as a breaking change.

One thing that might be interesting around types is proposed by @luisherranz in #62400. That PR proposes adding automated tests specifically for types. That could be extremely helpful for determining the types of changes in packages because any change that would make code using a previous versions types invalid would be a breaking change.

Thanks for the feedback. I capitalised it, made the type private, and used a simpler type.

One thing that might be interesting around types is proposed by @luisherranz in #62400.

Thank you for mentioning this, @sirreal.

Indeed, I want to propose a way to test types in Gutenberg. This PR shows how it could be done using tsc.

packages/dataviews/src/view-table.tsx

packages/dataviews/src/view-grid.tsx

ellatrix · 2024-06-25T15:51:05Z

packages/dataviews/src/view-list.tsx

-		[ onSelectionChange ]
-	);
+	const onSelect = ( item: Item ) =>
+		onSelectionChange( [ getItemId( item ) ] );


Note that useCallback is not needed here, it's just used in a React event callback down the line.

jorgefilipecosta · 2024-06-25T15:53:49Z

packages/dataviews/src/dataviews.tsx

-	);
+	const prevSelection = usePrevious( selection );
+
+	useEffect( () => {


Instead of an effect, what do you think of keeping exactly the other changes but instead of having an effect we create a callback with exactly the same signature as setSelection so all the other changes are valid, but that also calls the onSelectionChange inside and not only the setSelection? It would remove the need for the effect.

It's a bit less clean. Previously it was using a callback. If we move this all in a callback, it will pretty much re-render all the time based on these dependencies. I guess I can check if that callback was really needed in the first place.

Why do you not like the effect? Cause it could be triggered from outside selection changes? I think that would be expected?

Why do you not like the effect? Cause it could be triggered from outside selection changes? I think that would be expected?

Yah, the main difference with using an effect is that even if the consumer is the one doing the selection change the consumer will still get a callback on the onSelectionChange that may be unexpected. I'm not sure what is the good practice on these cases.

@jorgefilipecosta I changed it

jorgefilipecosta · 2024-06-26T11:40:25Z

Hi @ellatrix, I think there is a bug, on the page and templates table view I'm not able to select items by clicking on their checkbox.

packages/dataviews/src/dataviews.tsx

packages/dataviews/src/bulk-actions.tsx

oandregal · 2024-06-26T15:58:07Z

packages/dataviews/src/view-table.tsx

-							} )
-						);
-					}
+					onSelectionChange( ( state ) =>


Can all these calls that use the updater be rewritten as?

onSelectionChange( selection.includes(id) ? selection.filter((itemId) => id !== itemId) : [...selection, id] );

I haven't checked them all but it seems that where onSelectionChange is available, selection also is. If we can, it makes the updater redundant, and the API would simpler without it.

the API would simpler without it

I haven't changed the API at all, these are just React setState calls. Updater functions are nice because, if you're in memoized callbacks, you don't depend on the state, though I've removed all of those. It's also surfaced an issue: if you click on the checkbox, we're calling setState twice. This worked before because it would set state twice with the same value, but with the the updater, the second call is undoing the last call.

I see your point, thanks for elaborating 👍

Interesting finding regarding the duplicate setState call, I will have a look into that and see if I can fix it.

ellatrix · 2024-06-26T17:23:12Z

@jorgefilipecosta The bug is fixed, the cause was this: #62846 (comment)

Imo that should be fixed, we are calling setState twice, which is working in trunk because we don't use an updater function. But I think it's outside the scope of this PR so I changed it back to calling setState with the value.

ellatrix added the [Type] Code Quality Issues or PRs that relate to code quality label Jun 25, 2024

youknowriad reviewed Jun 25, 2024

View reviewed changes

packages/dataviews/src/view-table.tsx Show resolved Hide resolved

youknowriad reviewed Jun 25, 2024

View reviewed changes

packages/dataviews/src/view-grid.tsx Show resolved Hide resolved

ellatrix commented Jun 25, 2024

View reviewed changes

jorgefilipecosta reviewed Jun 25, 2024

View reviewed changes

ellatrix force-pushed the try/data-views-simplify-set-selection branch from 39b58a3 to b86375d Compare June 26, 2024 08:48

oandregal reviewed Jun 26, 2024

View reviewed changes

packages/dataviews/src/dataviews.tsx Outdated Show resolved Hide resolved

oandregal reviewed Jun 26, 2024

View reviewed changes

packages/dataviews/src/bulk-actions.tsx Show resolved Hide resolved

oandregal reviewed Jun 26, 2024

View reviewed changes

oandregal approved these changes Jun 27, 2024

View reviewed changes

DataViews: simplify selection setting

3b305c5

ellatrix force-pushed the try/data-views-simplify-set-selection branch from 7c92c8e to 3b305c5 Compare June 27, 2024 14:07

ellatrix enabled auto-merge (squash) June 27, 2024 14:08

ellatrix merged commit fe67010 into trunk Jun 27, 2024
60 checks passed

ellatrix deleted the try/data-views-simplify-set-selection branch June 27, 2024 14:43

github-actions bot added this to the Gutenberg 18.8 milestone Jun 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataViews: simplify selection setting #62846

DataViews: simplify selection setting #62846

ellatrix commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

youknowriad Jun 25, 2024

ellatrix Jun 25, 2024

youknowriad Jun 25, 2024

ellatrix Jun 25, 2024

jsnajdr Jun 25, 2024

sirreal Jun 25, 2024

ellatrix Jun 25, 2024

luisherranz Jun 26, 2024

ellatrix Jun 25, 2024

jorgefilipecosta Jun 25, 2024

ellatrix Jun 25, 2024

jorgefilipecosta Jun 25, 2024

ellatrix Jun 25, 2024

jorgefilipecosta commented Jun 26, 2024 •

edited

Loading

oandregal Jun 26, 2024

ellatrix Jun 26, 2024

oandregal Jun 27, 2024

jorgefilipecosta Jun 27, 2024

ellatrix commented Jun 26, 2024

DataViews: simplify selection setting #62846

DataViews: simplify selection setting #62846

Conversation

ellatrix commented Jun 25, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Jun 26, 2024

ellatrix commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

jorgefilipecosta commented Jun 26, 2024 •

edited

Loading