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

[core] Drop useGridContainerProps #3007

Merged
merged 97 commits into from
Nov 16, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Oct 27, 2021

Rebased upon #3074

Closes #1775

Changes

  • New method apiRef.current.getRootDimensions to get the current dimensions of the grid. Returns the following object
export interface GridDimensions {
  viewportOuterSize: ElementSize;
  viewportInnerSize: ElementSize;
  hasScrollX: boolean;
  hasScrollY: boolean;
}
  • New event viewportInnerSizeChange to re-compute the column flex widths and the autoPageSize

  • New method unstable_getViewportPageSize

  • Remove state.containerSizes, state.viewportSizes and state.scrollBar

Render count

I counted the amount of rerenders in GridBody when mounting a DataGrid without any further interaction.
Those renders can be synchronous and do not mean that React is actually committing anything to the DOM.

Async data fetching

const { data } = useDemoData({
    dataSet: 'Commodity',
    rowLength: 10,
    maxColumns: 6,
});

return (
    <div style={{ height: 400, width: '100%' }}>
        <DataGrid {...data} autoPageSize />
    </div>
);

Before: 12 renders
After: 5 renders

Sync data fetching with props.autoPageSize

const data = React.useMemo(() => getData(10, 10), [])

return (
    <div style={{ height: 400, width: '100%' }}>
        <DataGrid {...data} autoPageSize />
    </div>
);

Before: 7 renders
After: 2 renders

Sync data fetching without props.autoPageSize

const data = React.useMemo(() => getData(10, 10), [])

return (
    <div style={{ height: 400, width: '100%' }}>
        <DataGrid {...data} />
    </div>
);

Before: 7 renders
After: 2 renders

Why add this logic in useGridDimensions (ex useGridResizeContainer)

If we put this logic in GridVirtualScroller, then during the 1st render of the Rows and the Header, we don't have the dimensions yet. Which cuases flickering on the borders.

@flaviendelangle flaviendelangle self-assigned this Oct 27, 2021
@flaviendelangle flaviendelangle changed the title [core] Drop useGridContainerProps [core] Drop useGridContainerProps Oct 27, 2021
@flaviendelangle
Copy link
Member Author

I even removed virtualScrollerRowCount which was just passing a variable from useCurrentPageRows
We now only have the inner and outer dimensions

export interface GridDimensions {
  viewportOuterSize: ElementSize;
  viewportInnerSize: ElementSize;
  hasScrollX: boolean;
  hasScrollY: boolean;
}

clock = useFakeTimers(new Date('Mon Aug 18 14:11:54 2014 -0500'));
clock = useFakeTimers({
now: new Date('Mon Aug 18 14:11:54 2014 -0500').getTime(),
shouldAdvanceTime: true,
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise, in the e2e test you fake the timers so you never apply the setTimeout of debouncedResize and the dimensions ref is never set.

Copy link
Member Author

Choose a reason for hiding this comment

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

To give more context:
In useGridDimensions (and before in useGridContainerResize) we have the following logic:

const isTestEnvironment = process.env.NODE_ENV === 'test';

[...]

if (isTestEnvironment) {
  // We don't need to debounce the resize for tests.
  resize();
  return;
}

debounceResize();

Which solves allow to test post-resize result synchronously in all tests except the e2e / regressions
In e2e tests we have no test impacted to my knowledge.
In regressions it causes issues because we never set the dimensions ref and therefore never triggers behavior like the autoPageSize.

I spent a few hours trying to fix this. I can't get Webpack to use NODE_ENV === "test" because it wants to have its mode equal to the NODE_ENV and it only has development and production modes.
So I just removed the mock of the timer, I am now only mocking the initial time to have reliable snapshots but not how the time evolve.

Copy link
Member

Choose a reason for hiding this comment

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

I spent a few hours trying to fix this. I can't get Webpack to use NODE_ENV === "test" because it wants to have its mode equal to the NODE_ENV and it only has development and production modes.

It may be an useless question, but did you try webpack.DefinePlugin?

Copy link
Member Author

@flaviendelangle flaviendelangle Nov 15, 2021

Choose a reason for hiding this comment

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

Yes
If you put process.env.NODE_ENV: "test" it complains that the NODE_ENV should always be equal to the mode

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.

Actually I don't wanna approve yet. There're a few regressions pointed by argos.

The lighter horizontal scrollbar means that the overlay is on top of it. This means that users are trapped and can't scroll to the right. The vertical alignment of the text is also wrong.

image

From what I found, horizontalScrollbarSize begins as 0 and is never updated once the value is available.

https://github.com/mui-org/material-ui-x/blob/2409df34aa924b4dd0d2ef80d59f2812974a1171/packages/grid/_modules_/grid/components/containers/GridOverlay.tsx#L56

I believe fixing this also fixes the inconsistency in the infinite loading demo.

@flaviendelangle flaviendelangle dismissed m4theushw’s stale review November 15, 2021 10:05

There still is 2 Argos changes but I think those are normal. I fixed the overlay position.

{ "name": "GridDisableVirtualizationApi", "kind": "Interface" },
{ "name": "GridEditCellProps", "kind": "Interface" },
{ "name": "GridEditCellPropsParams", "kind": "Interface" },
{ "name": "GridEditCellValueParams", "kind": "Interface" },
{ "name": "GridEditRowApi", "kind": "Interface" },
{ "name": "GridEventsApi", "kind": "Interface" },
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced by GridDimensionsApi

@@ -114,7 +114,6 @@
{ "name": "GridRowTreeNodeConfig", "kind": "Interface" },
{ "name": "GridRowsState", "kind": "Interface" },
{ "name": "GridScrollApi", "kind": "Interface" },
{ "name": "GridScrollBarState", "kind": "Interface" },
Copy link
Member Author

Choose a reason for hiding this comment

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

This state has been removed

@@ -193,7 +192,6 @@
{ "name": "GridTranslationKeys", "kind": "Type alias" },
{ "name": "GridUpdateAction", "kind": "Type alias" },
{ "name": "GridValueGetterParams", "kind": "Type alias" },
{ "name": "GridViewportSizeState", "kind": "Type alias" },
Copy link
Member Author

Choose a reason for hiding this comment

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

This state has been removed

@@ -58,20 +58,20 @@
{ "name": "GridColumnsState", "kind": "Interface" },
{ "name": "GridCommitCellChangeParams", "kind": "Interface" },
{ "name": "GridComponentProps", "kind": "Interface" },
{ "name": "GridContainerProps", "kind": "Interface" },
Copy link
Member Author

Choose a reason for hiding this comment

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

This state has been removed

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! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Datagrid] The Grid flickers as it continuously resizes itself
3 participants