search

mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.5k stars 32.17k forks source link

[material-ui][Collapse] Deprecate `sx` prop #41251

Open DiegoAndai opened 7 months ago

DiegoAndai commented 7 months ago

Transitions shouldn't support the sx prop:

The Collapse transition is the only one with it in its types. We should deprecate it and mark it as to be removed. We should also add instructions on migrating on the migrating from deprecated APIs guide. We could also try adding a codemod, but I'm unsure how it should work.

Search keywords:

oliviertassinari commented 7 months ago

Transitions shouldn't support the sx prop:

Why not? It's not clear. I get why Transition, react-transition-group shouldn't document the sx prop, it's not a DOM node, so it doesn't host it (it should technically work though if the host supports it and types should warn against doing it). But the <Collapse> root hosts its own DOM node, so it looks like it should support the sx prop.

Here is another to look at it: if the className prop works (e.g. to add utility classes) then the sx prop should work. This looks like a baseline constraint to me, to never compromise on. I would actually love MUI System to implement the sx prop with class names: #23220.

I removed the "ready to take" label, it looks to me that we are at best all good. At worst, we miss the "Why" that we will write on the migration/deprecation docs page.

DiegoAndai commented 7 months ago

From the user's perspective, there's no difference between Collapse and the other transition components that don't host root DOM nodes. I would say the fact that Collapse hosts its own DOM node is an implementation detail that shouldn't translate to its API.

The downside to keeping the sx prop on Collapse is that it could lead to users expecting the other transition components to support it (https://github.com/mui/material-ui/pull/32025)—the mental model of how transition components work weakens.

It's not a huge issue, though. I see it as an API design enhancement rather than something that needs to be fixed.

What do you think @mnajdova?

oliviertassinari commented 7 months ago

I think that the problem is that it's very hard to make a single DOM node an implementation detail, each impacts the customization experience.

Joy UI has an alternative implementation that we could benchmark with, maybe there is room to remove this DOM node, but still, I don't see it as a clear step forward, Joy UI introduces noticeable regressions, the one I remember is with the animation duration.

On my end, the most important is that we lock sync the class name with the sx prop. For example, If we remove the sx prop, we should remove the class name prop.

DiegoAndai commented 1 month ago

As there is no consensus on this a the moment, and it's not a pressing issue either, I think we should move this out of the v6 milestone. Do you agree @mnajdova?