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] Components: merge icons in component slots #879

Merged
merged 13 commits into from
Jan 21, 2021

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Jan 19, 2021

One more iteration on #840

Breaking changes

  • [DataGrid] Move all the icon components overrides in the components prop. And added the suffix 'Icon' on each icon component. This change aims to bring consistency with the customization pattern of Material-UI v5:

    <DataGrid
    - icons: {{
    -   ColumnSortedAscending: SortedAscending,
    -  }},
    +  components={{
    +   ColumnSortedAscendingIcon: SortedAscending,
    +  }}
    />

Closes #840

@dtassone dtassone changed the title merge icons in components [DataGrid] Components: merge icons in component slots Jan 19, 2021
@dtassone dtassone requested review from oliviertassinari and DanailH and removed request for oliviertassinari January 19, 2021 19:17
Comment on lines 71 to 75
React.useEffect(() => {
if (apiRef && apiRef.current) {
apiRef.current.components = components;
}
}, [apiRef, components]);
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong. We update the apiRef in an effect, and yet, in the render, we read directly from it. No way it can work.

Copy link
Member

Choose a reason for hiding this comment

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

Why not use the context to propagate the components? The fewer we rely on global side effects (apiRef), the better.

Copy link
Member

@oliviertassinari oliviertassinari Jan 19, 2021

Choose a reason for hiding this comment

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

Yeah bingo, see "bug 1" in https://codesandbox.io/s/material-demo-forked-m7ilt?file=/demo.js. It only updates after we resize.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

By putting it on ApiRef it will be accessible anywhere. This allows us to render custom components from anywhere in the react tree.
Also by putting everything on ApiRef, users would have access to any prop using one single object as one single source of truth thus making the learning curve pretty easy.
Before I added multiple contexts, for rendering props and options, but I refactored as it's much easier to attach it to apiRef and pass it around.

Copy link
Member

@oliviertassinari oliviertassinari Jan 20, 2021

Choose a reason for hiding this comment

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

This fixes the reproduction :).

However, it also violates one of the rendering principles of React. React can render something and never commit it (update the DOM). This means that renders should be side-effect free. This is the opposite of what we do. We update a ref during a render, a state that can corrupt the next render. This is the mistake we/I did in @material-ui/styles and why it's not working in StrictMode, why we can't enable StrictMode in the docs for v4.

I think that this will likely require a re-architecture of the apiRef and state, it might come with #849. But for now, it's working so probably good enough, I also don't expect it to require a large refactoring in the future. It should be relatively straightforward. Sebastian would have probably never approved the changes 😁.

Also by putting everything on ApiRef, users would have access to any prop using one single object as one single source of truth thus making the learning curve pretty easy.

I don't understand the value for the developers and I'm worried about the cost for them.
From what I understand, by default!, we are making most of the "state"/"internals" of the data grid public. I have always thought the opposite to be a better strategy: make as much as we can private, be the default, and force developers to come up with problems so we can evaluate how best to solve them, expose public APIs when needed.
Will we be able to deliver large improvements in the data grid once we leave the alpha phase without requiring breaking changes each time, crippling our ability to deliver value?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@mnajdova Back a previous comment I left, what was your thought on the expected outcome of:

<DataGrid components={{ Footer: null }} />
// or
<DataGrid components={{ Footer: undefined }} />

Should it be equivalent to a noop component 1.

<DataGrid components={{ Footer: () => null }} />

or to the default component 2?

<DataGrid components={{ Footer: GridFooter }} />

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 20, 2021

@dtassone Could you update the pull request's description to list the breaking changes and the migration path as I did in #851? We use it to generate the changelog. It's important for developers to upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Implement components & componentsProps API
3 participants