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

[data grid] Row Selection: Select All - checkboxSelectionVisibleOnly doesn't accumulate selection after applying filters. #14074

Closed
1 task done
olzhas-kalikhan opened this issue Aug 1, 2024 · 7 comments · Fixed by #14677
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature feature: Selection Related to the data grid Selection feature regression A bug, but worse

Comments

@olzhas-kalikhan
Copy link

olzhas-kalikhan commented Aug 1, 2024

Latest version

  • I have tested the latest version

Steps to reproduce

Both in v6 and v7

Link to live example:
https://codesandbox.io/s/amazing-architecture-tn8d3h

Steps:

  1. Set checkboxSelectionVisibleOnly to true
  2. Apply any filter (make sure there is more than one page)
  3. Click Select All checkbox in frist page
  4. Go to the next page (or previous)
  5. Click Select All checkbox

ezgif-7-6ea2b21e8f

Current behavior

Previously selected rows are deselected and only rows in current page are selected, but if you select rows individually in the next page selected rows count is accumulated, and rows selected in previous pages are not deselected.

Expected behavior

Previously selected items shouldn't be deselected while filter is applied when clicking on select all checkbox

Context

Make consistent behaviour with vs without applied filters.
Context: After applying filter and selecting certain pages selection progress is lost because previously selected rows are deselected

Your environment

Forked docs demo from https://mui.com/x/react-data-grid/row-selection/#visible-rows-selection
Fork: https://codesandbox.io/s/amazing-architecture-tn8d3h

Search keywords: DataGrid, Filtered Row Selection

@olzhas-kalikhan olzhas-kalikhan added bug 🐛 Something doesn't work status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 1, 2024
@github-actions github-actions bot added the component: data grid This is the name of the generic UI component, not the React module! label Aug 1, 2024
@olzhas-kalikhan olzhas-kalikhan changed the title [DataGrid] Row Selection: Select All - Visible Only doesn't accumulate selection after applying filters. [DataGrid] Row Selection: Select All - checkboxSelectionVisibleOnly doesn't accumulate selection after applying filters. Aug 1, 2024
@michelengelen michelengelen changed the title [DataGrid] Row Selection: Select All - checkboxSelectionVisibleOnly doesn't accumulate selection after applying filters. [data grid] Row Selection: Select All - checkboxSelectionVisibleOnly doesn't accumulate selection after applying filters. Aug 2, 2024
@michelengelen michelengelen added feature: Filtering Related to the data grid Filtering feature feature: Selection Related to the data grid Selection feature labels Aug 2, 2024
@michelengelen
Copy link
Member

Hey @MBilalShafi ... Could this potentially be fixed with your recent PR #13757 ?

@MBilalShafi
Copy link
Member

MBilalShafi commented Aug 2, 2024

Hey @MBilalShafi ... Could this potentially be fixed with your recent PR #13757 ?

@michelengelen I think it's a separate problem, I'll see if it could be solved along with that one.

One notable thing that will change when propagateRowSelection is active is, the unfiltered rows will be automatically deselected on filtering. It could be previewed in this csb example.

@olzhas-kalikhan Do you see a problem with this approach as this is supposed to be the default selection behavior in v8? Do you want to keep the unfiltered rows selected after filtering?


Update:

the unfiltered rows will be automatically deselected on filtering.

This is not necessary for the row selection propagation to work. We could add it separately as an opt-in prop as mentioned in #14074 (comment)

@MBilalShafi MBilalShafi removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 3, 2024
@olzhas-kalikhan
Copy link
Author

correct me if I'm wrong, when filter everytime filter is applied all selections that don't match new filter will be deselected.
If that's the case, it makes sense.

There is another case where user might want to repeat filter + selection actions. eg Filter -> Select -> Filter ->Select. Im not sure if that will happen often let me know what you think. Some actions are made with this case is bulk edit or bulk download of records (some other actions like changing status of ticket, issuing invoice).

With the 1st approach it would be: Filter -> Select -> Bulk Action -> Filter -> Select -> Bulk Action -> Filter -> Select -> Bulk Action (Bulk Download - 3 files)
With the 2nd: Filter -> Select -> Filter -> Select -> Filter -> Select -> Bulk Action (Bulk Download - 1 file)

@olzhas-kalikhan
Copy link
Author

oh wait actually now Im thinking user can apply multiple filters to perform bulk action. Deselecting unfiltered rows should be fine

@MBilalShafi
Copy link
Member

MBilalShafi commented Aug 5, 2024

There is another case where user might want to repeat filter + selection actions. eg Filter -> Select -> Filter ->Select. Im not sure if that will happen often let me know what you think. Some actions are made with this case is bulk edit or bulk download of records (some other actions like changing status of ticket, issuing invoice).

Thank you for the feedback. This indeed looks like a valid use case. I imagine a use case with a large amount of data and the user wants to search and select things, they may likely want to keep the selection when filtering things on different criteria. I'll see how we can make room for such a use case.

@MBilalShafi
Copy link
Member

MBilalShafi commented Aug 19, 2024

This issue seems to be a regression introduced in #11751 while fixing another bug.
The solution done in that PR might be approached differently to avoid causing this regression, I'll explain the problem, solution, the problem with the solution, and possible alternate solution in the next section.

TL;DR

(Note: Recent update at the end)

Problem

The original problem in #11726 could be reproduced here: https://codesandbox.io/p/sandbox/wonderful-mccarthy-6dlcdc

Steps:

  1. Select all rows (Select all checkbox)
  2. Apply a filter to hide some rows
  3. Deselect all rows (Select all checkbox)

Some rows (now unfiltered) will still be selected in the background and couldn't be deselected.

Solution

On select all or deselect all, reset the rows state if some filter is applied. #11751 applies this solution.

Here's the line of code that causes the row selection model to reset if some filters are defined:

apiRef.current.selectRows(rowsToBeSelected, params.value, filterModel?.items.length > 0);

Problem with the solution

This solution causes a problem with checkboxSelectionVisibleOnly turned on.

Normally, when checkboxSelectionVisibleOnly is enabled, the select all checkbox should select all the visible rows of the current page, while keeping already selected rows on the other pages, which doesn't happen with filters defined because of resetting the previous selection model.

Check the example in the description to reproduce the issue.

Alternate solution

Instead of causing a reset selection when a filter is defined, we might introduce a prop keepUnfilteredRowsSelected (default false) to let the users control exactly how the row selection works with unfiltered rows.

Table below explains the behavior in different combinations:

checkboxSelectionVisibleOnly keepUnfilteredRowsSelected Apply Filters Select/Deselect All
true true Do nothing Select/Deselect current page rows
false true Do nothing Select/Deselect all rows
true false Deselect selected rows not fulfilling the filtering criteria Select/Deselect current page rows
false false Deselect selected rows not fulfilling the filtering criteria Select/Deselect all rows

Thoughts @mui/xgrid ?


Update

Starting without the prop keepUnfilteredRowsSelected and making false the default behavior makes sense since the use-case when this prop will be true has not been validated yet. We could introduce the prop later if we need to.

Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

We value your feedback @olzhas-kalikhan! How was your experience with our support team?
We'd love to hear your thoughts in this brief Support Satisfaction survey. Your insights help us improve!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature feature: Selection Related to the data grid Selection feature regression A bug, but worse
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants