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.

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

Add a “SELECT ALL” button in the <BulkActionsToolbar> #10367

Open
wants to merge 127 commits into
base: next
Choose a base branch
from

Conversation

erwanMarmelab
Copy link
Contributor

@erwanMarmelab erwanMarmelab commented Nov 20, 2024

Problem

The Datagrid already allows to select all the records of a given page. But sometimes users want to select all records across all pages, e.g. to delete all the records of a resource. Also, when not in a datagrid (e.g. contact list in CRM demo), there is no way to select all records.

Solution

In the bulk actions toolbar, add a “select all” link close to the number of selected items.

Already started in #9043, but needs a different approach

To do - V1

  • Add a "Select all" button
  • make it customizable (selectAllLimit)
  • document is the jsDoc
  • document
  • make it for the:
    • List
    • ReferenceManyField
    • ReferenceArrayField
    • InfiniteList
  • add stories:
  • change ra-core tests to transfor rendered component to stories

Informations

The limit prop is not usefull for the useList hook because this hook received all the data directly so we don't need to call it.

So, the <RerefenceArrayField> doesn't need the limit prop.

The "SELECT ALL" button will alway display when the API use a partial pagination.

To do - V2

  • Change the structure to only modify the new SelectAllButton for meta and limit
  • Document the new button
  • Test the new button

Additional Checks

  • The PR targets master for a bugfix, or next for a feature
  • The PR includes unit tests (if not possible, describe why)
  • The PR includes one or several stories (if not possible, describe why)
  • The documentation is up to date

@@ -168,6 +169,37 @@ export const useListController = <RecordType extends RaRecord = any>(
name: getResourceLabel(resource, 2),
});

const { data: allData } = useGetList<RecordType>(
Copy link
Member

Choose a reason for hiding this comment

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

You'd better use the data provider directly in the callback, or using react-query's useMutation. useGetList is really designed for queries triggered on mount.

Copy link
Collaborator

@djhi djhi left a comment

Choose a reason for hiding this comment

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

In every tests where you pass a children callback, you can instead modify the stories (or add new ones), adding a state inspector that shows the values you want to test stringified

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

We're getting closer! The UX and the DX are OK, now we have to clean up the code.

The `<SelectAllButton>` component allows users to select all items from a resource, no matter the pagination.

![SelectAllButton](./img/SelectAllButton.png)

Copy link
Member

Choose a reason for hiding this comment

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

I think we're missing a usage section here.

Comment on lines +395 to +396
By default, all Datagrids have a single `action`, [the select all button](./Buttons.md#selectallbutton).
You can customize this button or add others by passing a custom element as the `bulkActionsToolbar` prop of the `<Datagrid>` component:
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing. It's a bulk action toolbar, but it has an action prop that is not for bulk actions... I'd prefer that you add a selectAllButton prop for the SelectAllButton.


{% endraw %}

**Tip**: This customization will bring up your new `UnselectAll` button on the left-hand side of the toolbar. To work on the right-hand side, you can use the [`bulkActionButtons`](#bulkactionbuttons) prop.
Copy link
Member

Choose a reason for hiding this comment

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

Also unclear, especially since there is no screenshot of what's in the let and right sides.

@@ -373,6 +374,61 @@ const CustomResetViewsButton = () => {
};
```

## `bulkActionsToolbar`

You can use the `bulkActionsToolbar` prop to customize the bulk action toolbar, displayed when at least one row is selected.
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by reading this: why one should use bulkActionsToolbar instead of bulkActionButton? This should be clearer.

In fact, I suggest that we don't document this prop and keep it private for now.


export const BulkActionsToolbar = (props: BulkActionsToolbarProps) => {
const {
label = 'ra.action.bulk_actions',
children,
className,
actions = defaultActions,
Copy link
Member

Choose a reason for hiding this comment

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

rename the prop to selectAllButton

screen.queryByRole('button', { name: 'Select all' })
).toBeNull();
});
it('should not be displayed if we reached the limit by a manual selection', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should not be displayed if we reached the limit by a manual selection', async () => {
it('should not be displayed if the user reaches the limit by a manual selection', async () => {

screen.queryByRole('button', { name: 'Select all' })
).toBeNull();
});
it('should not be displayed if we reached the selectAllLimit by a click on the "Select all" button', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should not be displayed if we reached the selectAllLimit by a click on the "Select all" button', async () => {
it('should not be displayed if the user reaches the selectAllLimit by a click on the "Select all" button', async () => {

fireEvent.click(screen.getByRole('button', { name: 'Select all' }));
await screen.findByText('13 items selected');
});
it('should select the maximum items possible until we reached the selectAllLimit', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('should select the maximum items possible until we reached the selectAllLimit', async () => {
it('should select the maximum items possible up to the selectAllLimit', async () => {

Copy link
Member

Choose a reason for hiding this comment

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

some linter warnings in this file

* <BulkActionsToolbar actions={
* <>
* <SelectAllButton label="Select all records" />
* <UnselectButton />
Copy link
Member

Choose a reason for hiding this comment

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

This example doesn't make sense, as we already have an unselect button (the cross). I suggest you create a custom toolbar that doesn't use BulkActionsToolbar, but simply lists the selected ids

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants