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

[DataGrid] Make GRID_ROWS_CLEAR private #1925

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 18, 2021

Breaking changes


See the rationale in #1862 (comment). When GRID_ROWS_CLEARED triggers, GRID_ROWS_SET is also triggered, no need to update the sorting state twice.

Note that this makes GRID_ROWS_CLEARED an orphan, nobody is listening to the event.

If I'm missing something and is really required, then it surfaces a hole in the test suite, the CI is green.

@dtassone
Copy link
Member

I don't think it is dead code at least when I wrote it, it was not.
If I remember well it is to clear the sortedRows used sortedGridRowIdsSelector, to avoid getting an error when a row was added or removed and not in the lookup.

@dtassone
Copy link
Member

I don't think it is dead code at least when I wrote it, it was not.
If I remember well it is to clear the sortedRows used sortedGridRowIdsSelector, to avoid getting an error when a row was added or removed and not in the lookup.

I think the rendering optimisation fixed it

@oliviertassinari oliviertassinari changed the title [core] Remove dead code [core] Remove legacy code Jun 18, 2021
@oliviertassinari oliviertassinari changed the title [core] Remove legacy code [core] Remove legacy logic Jun 18, 2021
@oliviertassinari oliviertassinari changed the title [core] Remove legacy logic [DataGrid] Remove GRID_ROWS_CLEARED event Jun 18, 2021
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

This PR broke https://deploy-preview-1925--material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-filter--commodity-with-new-rows-via-api

The following diff passes on HEAD but here doesn't:

diff --git a/packages/grid/x-grid/src/tests/rows.XGrid.test.tsx b/packages/grid/x-grid/src/tests/rows.XGrid.test.tsx
index c800f9cb..591610b2 100644
--- a/packages/grid/x-grid/src/tests/rows.XGrid.test.tsx
+++ b/packages/grid/x-grid/src/tests/rows.XGrid.test.tsx
@@ -162,6 +162,7 @@ describe('<XGrid /> - Rows', () => {

     it('should allow to reset rows with setRows and render after 100ms', () => {
       render(<TestCase />);
+      expect(getColumnValues()).to.deep.equal(['Nike', 'Adidas', 'Puma']);
       const newRows = [
         {
           id: 3,
@@ -169,10 +170,9 @@ describe('<XGrid /> - Rows', () => {
         },
       ];
       apiRef.current.setRows(newRows);
-
-      clock.tick(50);
-      expect(getColumnValues()).to.deep.equal(['Nike', 'Adidas', 'Puma']);
-      clock.tick(50);
+      // Force an update before the 100ms
+      apiRef.current.forceUpdate(() => apiRef.current.state);
+      clock.tick(100);
       expect(getColumnValues()).to.deep.equal(['Asics']);
     });

My suggestion is to try to remove the timeout from the custom forceUpdate in useGridRows or to reset the sorted rows after updating the rows in this hook (might break the separation of concern).

@oliviertassinari oliviertassinari changed the title [DataGrid] Remove GRID_ROWS_CLEARED event [DataGrid] Make GRID_ROWS_CLEARED private Jun 20, 2021
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 20, 2021

This PR broke

@m4theushw Oh wow, great catch. I have added a failing test case to avoid regressions in the future. I have also updated the assertion to illustrate the current behavior. I have reduced the ambition of the changes, I don't have the bandwidth to improve the current state more that this.

@dtassone
Copy link
Member

Should we use this PR to rename the event to match the convention CLEARED => CLEAR?

@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Jun 21, 2021
@oliviertassinari
Copy link
Member Author

I'm waiting for #1862 to be merged to rebase.

@oliviertassinari oliviertassinari removed on hold There is a blocker, we need to wait performance labels Jun 21, 2021
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jun 21, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid] Make GRID_ROWS_CLEARED private [DataGrid] Make GRID_ROWS_CLEAR private Jun 21, 2021
@oliviertassinari oliviertassinari merged commit 067c6dc into mui:master Jun 22, 2021
@oliviertassinari oliviertassinari deleted the fitler-clear-dead-code branch June 22, 2021 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants