Skip to content
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: Support passing the registry to actions callbacks #62505

Merged
merged 5 commits into from
Jun 18, 2024
Merged

DataViews: Support passing the registry to actions callbacks #62505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1f0325f
DataViews: Support thunk callbacks in actions
youknowriad Jun 12, 2024
056a66e
Update DataViews changelog
youknowriad Jun 12, 2024
80ed76b
Fix static checks
youknowriad Jun 12, 2024
bc8a515
Simplify API
youknowriad Jun 12, 2024
b8996d6
Clarify changelog
youknowriad Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/dataviews/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
### Enhancement

- `label` prop in Actions API can be either a `string` value or a `function`, in case we want to use information from the selected items. ([#61942](https://github.com/WordPress/gutenberg/pull/61942)).
- Add `registry` argument to the callback of the actions API. ([#62505](https://github.com/WordPress/gutenberg/pull/62505)).

## 1.2.0 (2024-05-16)

Expand Down
1 change: 1 addition & 0 deletions packages/dataviews/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"@babel/runtime": "^7.16.0",
"@wordpress/components": "file:../components",
"@wordpress/compose": "file:../compose",
"@wordpress/data": "file:../data",
"@wordpress/element": "file:../element",
"@wordpress/i18n": "file:../i18n",
"@wordpress/icons": "file:../icons",
Expand Down
6 changes: 5 additions & 1 deletion packages/dataviews/src/bulk-actions-toolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { useMemo, useState, useRef } from '@wordpress/element';
import { _n, sprintf, __ } from '@wordpress/i18n';
import { closeSmall } from '@wordpress/icons';
import { useReducedMotion } from '@wordpress/compose';
import { useRegistry } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -92,6 +93,7 @@ function ActionButton< Item extends AnyItem >( {
actionInProgress,
setActionInProgress,
}: ActionButtonProps< Item > ) {
const registry = useRegistry();
const selectedEligibleItems = useMemo( () => {
return selectedItems.filter( ( item ) => {
return ! action.isEligible || action.isEligible( item );
Expand All @@ -113,7 +115,9 @@ function ActionButton< Item extends AnyItem >( {
action={ action }
onClick={ () => {
setActionInProgress( action.id );
action.callback( selectedItems );
action.callback( selectedItems, {
registry,
} );
} }
items={ selectedEligibleItems }
isBusy={ actionInProgress === action.id }
Expand Down
4 changes: 3 additions & 1 deletion packages/dataviews/src/bulk-actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@wordpress/components';
import { __, sprintf, _n } from '@wordpress/i18n';
import { useMemo, useState, useCallback, useEffect } from '@wordpress/element';
import { useRegistry } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -119,6 +120,7 @@ function BulkActionItem< Item extends AnyItem >( {
selectedItems,
setActionWithModal,
}: BulkActionsItemProps< Item > ) {
const registry = useRegistry();
const eligibleItems = useMemo( () => {
return selectedItems.filter(
( item ) => ! action.isEligible || action.isEligible( item )
Expand All @@ -136,7 +138,7 @@ function BulkActionItem< Item extends AnyItem >( {
if ( shouldShowModal ) {
setActionWithModal( action );
} else {
await action.callback( eligibleItems );
action.callback( eligibleItems, { registry } );
}
} }
suffix={
Expand Down
18 changes: 10 additions & 8 deletions packages/dataviews/src/item-actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
import { __ } from '@wordpress/i18n';
import { useMemo, useState } from '@wordpress/element';
import { moreVertical } from '@wordpress/icons';
import { useRegistry } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -116,12 +117,7 @@ export function ActionModal< Item extends AnyItem >( {
action.id
) }` }
>
<action.RenderModal
items={ items }
closeModal={ closeModal }
onActionStart={ action.onActionStart }
onActionPerformed={ action.onActionPerformed }
/>
<action.RenderModal items={ items } closeModal={ closeModal } />
</Modal>
);
}
Expand Down Expand Up @@ -159,6 +155,7 @@ export function ActionsDropdownMenuGroup< Item extends AnyItem >( {
actions,
item,
}: ActionsDropdownMenuGroupProps< Item > ) {
const registry = useRegistry();
return (
<DropdownMenuGroup>
{ actions.map( ( action ) => {
Expand All @@ -176,7 +173,9 @@ export function ActionsDropdownMenuGroup< Item extends AnyItem >( {
<DropdownMenuItemTrigger
key={ action.id }
action={ action }
onClick={ () => action.callback( [ item ] ) }
onClick={ () => {
action.callback( [ item ], { registry } );
} }
items={ [ item ] }
/>
);
Expand All @@ -190,6 +189,7 @@ export default function ItemActions< Item extends AnyItem >( {
actions,
isCompact,
}: ItemActionsProps< Item > ) {
const registry = useRegistry();
const { primaryActions, eligibleActions } = useMemo( () => {
// If an action is eligible for all items, doesn't need
// to provide the `isEligible` function.
Expand Down Expand Up @@ -233,7 +233,9 @@ export default function ItemActions< Item extends AnyItem >( {
<ButtonTrigger
key={ action.id }
action={ action }
onClick={ () => action.callback( [ item ] ) }
onClick={ () => {
action.callback( [ item ], { registry } );
} }
items={ [ item ] }
/>
);
Expand Down
20 changes: 7 additions & 13 deletions packages/dataviews/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,28 +327,16 @@ interface ActionBase< Item extends AnyItem > {

export interface ActionModal< Item extends AnyItem >
extends ActionBase< Item > {
/**
* The callback to execute when the action has finished.
*/
onActionPerformed: ( ( items: Item[] ) => void ) | undefined;

/**
* The callback to execute when the action is triggered.
*/
onActionStart: ( ( items: Item[] ) => void ) | undefined;

/**
* Modal to render when the action is triggered.
*/
RenderModal: ( {
items,
closeModal,
onActionStart,
onActionPerformed,
}: {
items: Item[];
closeModal?: () => void;
onActionStart?: ( items: Item[] ) => void;
onActionPerformed?: ( items: Item[] ) => void;
} ) => ReactElement;

Expand All @@ -368,7 +356,13 @@ export interface ActionButton< Item extends AnyItem >
/**
* The callback to execute when the action is triggered.
*/
callback: ( items: Item[] ) => void;
callback: (
items: Item[],
context: {
registry: any;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the point of view of a consumer that wants to use the @wordpress/dataviews package in its own screens, what's a registry?

I lack context on the way we are evolving actions and how this helps, so I'd like to learn more why we want to tie wordpress/dataviews to wordpress/data concepts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @youknowriad @ntsekouras @jsnajdr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registry just allows to any wordpress store. It doesn't introduce a dependency to a particular store but it allows dataviews actions to trigger actions from any store.

We used to that by having "React hooks" that actually return action but that is not ideal IMO. I prefer for dataviews actions to be global registrable objects just like blocks for instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes I understand the complaint and I think it's very valid 🙂 This PR makes the dataviews library dependent on data, even though it doesn't really use it. It just retrieves the registry and passes it to actions. And most actions won't use the registry at all. But the data library still needs to be present because the useRegistry calls are happening all the time.

A much more flexible approach would be if the context was empty by default, and you would configure it when creating a dataview. I'm not very familiar with dataviews, but something like this might work:

const registry = useRegistry();
const actionsContext = useMemo( () => ( { registry } ), [ registry ] ); 
  
return <DataViews
  actions={ actions }
  actionsContext={ actionsContext }
/>

A certain app like edit-site wants to pass registry to its actions, so it configures its DataViews accordingly. The library is completely unaware about what's in the context, and other apps can put something completely different there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jarda, your example illustrates a lot better the point, thanks. I still like to clarify why the "context" needs to be passed through the DataViews API if it is not used/modified by it. Cannot the actions retrieve whatever extra data they need?

const actions = [
  {
    id: 'view',
    callback: ( items, { onActionPerformed }  ) => {
      // onActionPerformed is triggered by DataViews. 
      // The action is responsible to retrieve whatever else it needs.
    }
  }
];
return <DataViews
   actions={ actions }
  />

I understand a valid use case for having to pass something through would be the following: the action's callback needs to have something ready when it is called. Is this the case? If so, could we do something along these lines instead?

const registry = useRegistry();
const actions = [
  {
    id: 'view',
    callback: ( items, { onActionPerformed, callbackContext }  ) => {
      // onActionPerformed is triggered by DataViews. 
      // callbackContext is the action's responsibility to define, but DataViews passes it through.
    },
    callbackContext: {
      registry
    }
  }
];
return <DataViews
   actions={ actions }
  />

I reckon this is still part of what DataViews has to do (passing the callbackContext to the callback), though the locality makes it easier to reason about (it feels it's part of the Actions API and not of DataViews API) — with the added benefit that each action can have a different context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously, it's possible to call the registration action within a hook as well but it does make extensibility a bit harder. When should a third-party script "unregister" a default provided core action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't like as well, is that it will actually forces us to retriggers the whole registration action when a selected value used with isElligible is resolved (for instance, when the active rendering mode changes, or active post type, or user permission...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like actions and fields to be able to be registered a bit more declaratively

Yes this sounds similar to what I also write above: often it's a good idea to define actions and the context separately and independently. It's then up to framework to inject all the dependencies (context) when calling the action.

It's worth exploring passing the context (or context creator function) to DataViews as prop.

Is the context passed to isEligible different from the context passed to the action callback?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, that I think about it more, there's a use-case that can't really be solved with the approach suggested #62505 (comment)

Show/hide an action by checking the permissions for the current item(s). Permissions changes from one row to the other in a data view, so how would you define an action who isEligible call should use a selector value ( canUser ). The only possibility I see is to register/unregister the action when the selection changes. It doesn't seem very ergonomic or DevX friendly to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really the same, but a similar sacrifice was made here: #60724

onActionPerformed?: ( items: Item[] ) => void;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the onActionPerformed field used anywhere? I see that all calls pass just { registry }.

Is it possible to make context the first argument? It's something like state for a selector or a this argument when defining type for a TS function. It will look odd when you later add a third or four parameter and context will be randomly somewhere in the middle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense to put the context as the last argument since it's not really necessary for the callback. The "items" is more important.

Also, one of the reasons, I made it an object is to be able to add more optional configs later there.

For the onActionPerformed, yes it's used in both post and site editors (but not in dataviews). I don't love this API personally, but I don't want to change everything here. It can potentially be made unnecessary in callback actions by making these functions return promises but it will be necessary in Modal Actions as a prop, so maybe it's fine to keep it for callback actions as an argument too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside is that if you ever add new parameters (how likely is that?) then context will have to move or will end up in a weird position. 🤷‍♂️

) => void;
}

export type Action< Item extends AnyItem > =
Expand Down
13 changes: 8 additions & 5 deletions packages/dataviews/src/view-list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
} from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { moreVertical } from '@wordpress/icons';
import { useRegistry } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -67,6 +68,7 @@ function ListItem< Item extends AnyItem >( {
store,
visibleFields,
}: ListViewItemProps< Item > ) {
const registry = useRegistry();
const itemRef = useRef< HTMLElement >( null );
const labelId = `${ id }-label`;
const descriptionId = `${ id }-description`;
Expand Down Expand Up @@ -235,11 +237,12 @@ function ListItem< Item extends AnyItem >( {
primaryAction.isDestructive
}
size="compact"
onClick={ () =>
primaryAction.callback( [
item,
] )
}
onClick={ () => {
primaryAction.callback(
[ item ],
{ registry }
);
} }
/>
}
/>
Expand Down
1 change: 1 addition & 0 deletions packages/dataviews/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"references": [
{ "path": "../components" },
{ "path": "../compose" },
{ "path": "../data" },
{ "path": "../element" },
{ "path": "../i18n" },
{ "path": "../icons" },
Expand Down
Loading
Loading