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

[DataGrid] Persist selection from selectionModel when page changes #1864

Closed
wants to merge 3 commits into from
Closed

[DataGrid] Persist selection from selectionModel when page changes #1864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f37e441
Reapply selectionModel on pageChage
DanailH Jun 10, 2021
65d45e0
Add tests
DanailH Jun 10, 2021
d5ec296
Simplify solution and update tests
DanailH Jun 15, 2021
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
10 changes: 7 additions & 3 deletions packages/grid/_modules_/grid/hooks/features/selection/gridSelectionSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,19 @@ import { gridRowsLookupSelector, GridRowsLookup } from '../rows/gridRowsSelector
import { GridState } from '../core/gridState';
import { GridSelectionState } from './gridSelectionState';
import { GridRowId, GridRowModel } from '../../../models/gridRows';
import { optionsSelector } from '../../utils/optionsSelector';
import { GridOptions } from '../../../models/gridOptions';

export const gridSelectionStateSelector = (state: GridState) => state.selection;
export const selectedGridRowsCountSelector: OutputSelector<
GridState,
number,
(res: GridSelectionState) => number
> = createSelector<GridState, GridSelectionState, number>(
(res: GridSelectionState, options: GridOptions) => number
> = createSelector<GridState, GridSelectionState, GridOptions, number>(
gridSelectionStateSelector,
(selection) => Object.keys(selection).length,
optionsSelector,
(selection, options) =>
options.selectionModel ? options.selectionModel.length : Object.keys(selection).length,
Copy link
Member

@dtassone dtassone Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

you should not use the options.selectionModel here as the grid read the state. So the state is the source of truth and should reflect what is in option.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the selectionModel is not stored in the state right? From what I understood if the selectionModel is provided then that should be the one that is used.

Copy link
Member

Choose a reason for hiding this comment

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

Yes so if you set the model it updates the state.selection, then if you select a row, the state will be updated. Therefore, the source of truth is state.selection

Copy link
Member

Choose a reason for hiding this comment

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

you should not use the options.selectionModel here as the grid read the state.

We don't access the options inside any of the selectors. I would check if it's in controlled mode in the hook.

From what I understood if the selectionModel is provided then that should be the one that is used.

Yeah, I was expecting the same. But instead of accessing the prop directly I would make the state to reflect the value of the prop, that means not updating the state directly when the selection changes.

Copy link
Member

@oliviertassinari oliviertassinari Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I can understand why we would want to keep the selector state only, we have never used the options before, it can feel scary, a new pattern. 👍 for trying a solution without.

For the prop updating the state or not. In https://github.com/mui-org/material-ui/blob/2c4dc994da164a53b253f03bcd2425a52655f175/packages/material-ui-utils/src/useControlled.js#L8 we skip the state. However, here, we have a double notion of prop controlled and state-controlled, with the state with more priority. So we probably want to update the state (as done in the pagination).

);

export const selectedGridRowsSelector = createSelector<
Expand Down
23 changes: 20 additions & 3 deletions packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ export const useGridSelection = (apiRef: GridApiRef): void => {
logger.debug(`Selecting row ${id}`);

setGridState((state) => {
if (selectionModel) {
return state;
}

let selectionState: GridSelectionState = { ...state.selection };
const allowMultiSelect =
allowMultipleOverride ||
Expand Down Expand Up @@ -94,8 +98,16 @@ export const useGridSelection = (apiRef: GridApiRef): void => {
data: row,
isSelected: selectionState[id] !== undefined,
};

let newSelectionModel = Object.values(selectionState);
if (selectionModel) {
newSelectionModel = selectionModel.includes(rowModelParams.id)
? selectionModel.filter((selection) => selection !== rowModelParams.id)
: [...selectionModel, rowModelParams.id];
}
Comment on lines +102 to +107
Copy link
Member

Choose a reason for hiding this comment

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

Wait, are we implementing the logic that is inside the above setGridState twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it looks like it but it is much simpler. That one only appends the newly selected row id to the already existing selection model so that the value of the params that the developer will get from the onSelectionModelChange is correct. There is no state or options update.

Copy link
Member

@oliviertassinari oliviertassinari Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

but it is much simpler

Being simpler is suspicious. What about multi-selection? Does it still work correctly? I went on trying it out. The answer is NO, it breaks it: https://codesandbox.io/s/material-demo-forked-e0puo?file=/demo.js.

It's the same root problem as https://github.com/mui-org/material-ui-x/pull/1864/files#r652274738.

so that the value of the params that the developer will get from the onSelectionModelChange is correct

It seems that we need the exact same output as in setGridState, so that we should enforce it with a shared abstraction?

One thing that is counterintuitive to me is why we don't have the exact same structure between the controlled prop and the state we store. If the why is performance, caching, then this value stored in the state should be derived from the source of truth (the controlled prop structure), not become the source of truth as it's now.

cc @dtassone what do you think? I'm asking because it resonates with the ongoing effort to structure more controlled implementation.

Copy link
Member

Choose a reason for hiding this comment

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

We use an object for the selection state to avoid looping through an array to know if a row is selected, otherwise, if you have 1000k rows we would have a perf hit.
https://github.com/mui-org/material-ui-x/blob/master/packages/grid/_modules_/grid/components/GridViewport.tsx#L80

However, for the prop, using an object is an option but it seemed to make less sense, and it is more convenient to use an array.

Copy link
Member

@oliviertassinari oliviertassinari Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@dtassone The caching makes sense, it definitely seems valid.

What do you think about we stored the controlled prop as is, in the store, and store a private cache, a derivated value, in another part of the state (one that we wouldn't expose to the developers that want to store the state in their database ). But more importantly, have updates on the state.selectionModel being the source of truth, and triggering changes automatically, to its cache, say state.cache.selection (for performance on the read)?

Copy link
Member

Choose a reason for hiding this comment

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

Well the controlled prop to me is already stored and is the value of the prop itself. Why would we need to duplicate the values?
And the private cache is actually the grid state and it is the source of truth.
We could do a selector that transforms the state to make it more storable.

Copy link
Member

@oliviertassinari oliviertassinari Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why would we need to duplicate the values?

To fix the very first comment of this thread.

the controlled prop to me is already stored and is the value of the prop itself.

The controlled prop selectionModel is transformed in https://github.com/mui-org/material-ui-x/blob/1ff0840017274576207208287ffc1f25934b8c56/packages/grid/_modules_/grid/hooks/features/selection/useGridSelection.ts#L217-L222

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I was looking in that use case yesterday and what I struggle to understand is how would you differentiate multiselection using CTRL from the normal one? The only way I see it is that if it is not multi-selection then basically every time the onSelectionModelChange is called we need to always pass just one ID (the lasted one)?

Then for the muti-selection and checkbox selection, you would append/remove IDs based on what is being selected/deselected.

Copy link
Member

@dtassone dtassone Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Considering #1823 and the equality issue and the discussion here, how about we transform state to be optimized in the selector. As they are memoized it will only do the transformation when the underlying model change. That way it would allow us to store the model in the state, fix the equality issue #1823 and avoid model duplication.
The cache mentioned by @oliviertassinari would actually be provided by the selector memoization. What do you think?

Copy link
Member

@oliviertassinari oliviertassinari Jun 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

we transform state to be optimized in the selecto

@dtassone This could work if we also expose the raw state. We need the raw state to perform the control feedback loop.

how would you differentiate multiselection using CTRL from the normal one?

@DanailH I'm not following. Is "you" a maintainer or a developer? But in any case, I don't understand the problem. Developers don't need to differentiate multiselection, they get the whole state. And as a maintainer, we have access to rowModelParams to differentiate multiselection.


const selectionChangeParam: GridSelectionModelChangeParams = {
selectionModel: Object.values(selectionState),
selectionModel: newSelectionModel,
};
apiRef.current.publishEvent(GRID_ROW_SELECTED, rowSelectedParam);
apiRef.current.publishEvent(GRID_SELECTION_CHANGED, selectionChangeParam);
Expand All @@ -106,6 +118,7 @@ export const useGridSelection = (apiRef: GridApiRef): void => {
apiRef,
logger,
checkboxSelection,
selectionModel,
forceUpdate,
setGridState,
],
Expand Down Expand Up @@ -198,10 +211,14 @@ export const useGridSelection = (apiRef: GridApiRef): void => {

React.useEffect(() => {
setGridState((state) => {
if (selectionModel) {
return state;
}
Comment on lines +214 to +216
Copy link
Member

Choose a reason for hiding this comment

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

The two changes in this useEffect looks KO. The behavior should be controlled invariant. Meaning, if the uncontrolled selection unselects when the rows changes, then the controlled version of the selection should behave identically (unselect).

Copy link
Member

Choose a reason for hiding this comment

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

Here, we stop unselecting if controlled 💥


const newSelectionState = { ...state.selection };
DanailH marked this conversation as resolved.
Show resolved Hide resolved
let hasChanged = false;
Object.keys(newSelectionState).forEach((id: GridRowId) => {
if (!rowsLookup[id]) {
if (!rowsLookup[id] || !selectionModel) {
delete newSelectionState[id];
hasChanged = true;
}
Expand All @@ -212,7 +229,7 @@ export const useGridSelection = (apiRef: GridApiRef): void => {
return state;
});
forceUpdate();
}, [rowsLookup, apiRef, setGridState, forceUpdate]);
}, [rowsLookup, selectionModel, apiRef, setGridState, forceUpdate]);

React.useEffect(() => {
const currentModel = Object.values(apiRef.current.getState().selection);
Expand Down
66 changes: 66 additions & 0 deletions packages/grid/data-grid/src/tests/selection.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,72 @@ describe('<DataGrid /> - Selection', () => {
expect(getRow(0)).not.to.have.class('Mui-selected');
expect(getRow(1)).not.to.have.class('Mui-selected');
});

it('should apply selectionModel on page change', () => {
render(
<div style={{ width: 300, height: 300 }}>
<DataGrid
rows={[
{
id: 0,
brand: 'Nike',
},
{
id: 1,
brand: 'Adidas',
},
{
id: 2,
brand: 'Puma',
},
]}
columns={[{ field: 'brand' }]}
selectionModel={[0]}
pageSize={2}
/>
</div>,
);

fireEvent.click(screen.getByRole('cell', { name: 'Adidas' }));

fireEvent.click(screen.getByRole('button', { name: /next page/i }));
fireEvent.click(screen.getByRole('button', { name: /previous page/i }));

const firstRow = getRow(0);
expect(firstRow).to.have.class('Mui-selected');

const secondRow = getRow(1);
expect(secondRow).not.to.have.class('Mui-selected');
});

it('should pass the new id to the onSelectionModelChange props', () => {
const handleSelectionModelChange = spy();
render(
<div style={{ width: 300, height: 300 }}>
<DataGrid
rows={[
{
id: 0,
brand: 'Nike',
},
{
id: 1,
brand: 'Adidas',
},
]}
columns={[{ field: 'brand' }]}
selectionModel={[0]}
pageSize={1}
onSelectionModelChange={handleSelectionModelChange}
/>
</div>,
);

fireEvent.click(screen.getByRole('button', { name: /next page/i }));
fireEvent.click(screen.getByRole('cell', { name: 'Adidas' }));

expect(handleSelectionModelChange.lastCall.args[0].selectionModel).to.deep.equal([0, 1]);
});
});

describe('props: isRowSelectable', () => {
Expand Down
5 changes: 3 additions & 2 deletions packages/grid/x-grid/src/tests/selection.XGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ describe('<XGrid /> - Selection', () => {
});
});

it('should clean the selected ids when the rows prop changes', () => {
it('should clean the selection when the rows prop changes and no selectionModel is provided', () => {
const { setProps } = render(<Test selectionModel={[0, 1, 2]} checkboxSelection />);
expect(apiRef.current.getSelectedRows()).to.have.keys([0, 1, 2]);
setProps({
Expand All @@ -123,7 +123,8 @@ describe('<XGrid /> - Selection', () => {
brand: 'Nike',
},
],
selectionModel: undefined,
});
expect(apiRef.current.getSelectedRows()).to.have.keys([0]);
expect(Object.values(apiRef.current.getSelectedRows().keys())).to.deep.equal([]);
});
});