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] Fix flickering on mount #7205

Merged
merged 4 commits into from Dec 27, 2022

Conversation

cherniavskii
Copy link
Member

Port of #7155 to the next branch

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine design: ux labels Dec 14, 2022
@mui-bot
Copy link

mui-bot commented Dec 14, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-7205--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 674 1,221.3 674 930.22 205.736
Sort 100k rows ms 693.3 1,301.6 1,301.6 1,004.76 199.959
Select 100k rows ms 175.8 330.6 284.2 268.34 56.074
Deselect 100k rows ms 201.8 319.5 212.8 235 43.346

Generated by 🚫 dangerJS against 260478a

@@ -1121,7 +1121,7 @@ describe('<DataGridPro /> - Row Editing', () => {
const cell = getCell(0, 2);
userEvent.mousePress(cell);
fireEvent.doubleClick(cell);
await act(() => {
await waitFor(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why, but without this change the test was failing with:

The current testing environment is not configured to support act

See https://app.circleci.com/pipelines/github/mui/mui-x/30074/workflows/a9f04000-72a1-436d-819b-360e41eabb40/jobs/172385

Copy link
Member

@m4theushw m4theushw Dec 22, 2022

Choose a reason for hiding this comment

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

I went through the call stack and found that the column headers are trying to react to a scroll event. Placing a breakpoint where scrollPositionChange is published I found that the custom edit component is calling .focus(). You can revert this change and do the following to make the test pass:

diff --git a/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx b/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx
index 34cf0badc..061277b03 100644
--- a/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx
+++ b/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx
@@ -27,7 +27,7 @@ describe('<DataGridPro /> - Row Editing', () => {
     const ref = React.useRef<HTMLInputElement>(null);
     React.useLayoutEffect(() => {
       if (hasFocus) {
-        ref.current!.focus();
+        ref.current!.focus({ preventScroll: true });
       }
     }, [hasFocus]);
     return <input ref={ref} />;

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, Matheus!

@@ -716,13 +716,13 @@ describe('<DataGrid /> - Keyboard', () => {
<DataGrid rows={rows} columns={columns} />
</div>,
);
expect(renderCell.callCount).to.equal(2);
expect(renderCell.callCount).to.equal(6);
Copy link
Member Author

Choose a reason for hiding this comment

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

And the number of rerenders changed here again 🤷🏻‍♂️
#7155 (comment)

@cherniavskii cherniavskii marked this pull request as ready for review December 21, 2022 15:42
@@ -1121,7 +1121,7 @@ describe('<DataGridPro /> - Row Editing', () => {
const cell = getCell(0, 2);
userEvent.mousePress(cell);
fireEvent.doubleClick(cell);
await act(() => {
await waitFor(() => {
Copy link
Member

@m4theushw m4theushw Dec 22, 2022

Choose a reason for hiding this comment

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

I went through the call stack and found that the column headers are trying to react to a scroll event. Placing a breakpoint where scrollPositionChange is published I found that the custom edit component is calling .focus(). You can revert this change and do the following to make the test pass:

diff --git a/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx b/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx
index 34cf0badc..061277b03 100644
--- a/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx
+++ b/packages/grid/x-data-grid-pro/src/tests/rowEditing.DataGridPro.test.tsx
@@ -27,7 +27,7 @@ describe('<DataGridPro /> - Row Editing', () => {
     const ref = React.useRef<HTMLInputElement>(null);
     React.useLayoutEffect(() => {
       if (hasFocus) {
-        ref.current!.focus();
+        ref.current!.focus({ preventScroll: true });
       }
     }, [hasFocus]);
     return <input ref={ref} />;

@cherniavskii cherniavskii merged commit 2b55e92 into mui:next Dec 27, 2022
@cherniavskii cherniavskii deleted the fix-grid-flickering-next branch December 27, 2022 13:36
@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer and removed design: ux labels Aug 18, 2023
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! design This is about UI or UX design, please involve a designer feature: Rendering layout Related to the data grid Rendering engine v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants