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

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

Open
DiegoAndai opened this issue Feb 23, 2024 · 4 comments
Open

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

DiegoAndai opened this issue Feb 23, 2024 · 4 comments
Labels
component: Collapse The React component deprecation New deprecation message package: material-ui Specific to @mui/material

Comments

@DiegoAndai
Copy link
Member

DiegoAndai commented Feb 23, 2024

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:

@DiegoAndai DiegoAndai added component: Collapse The React component deprecation New deprecation message package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Feb 23, 2024
@DiegoAndai DiegoAndai added this to the Material UI: v6 milestone Feb 23, 2024
@DiegoAndai DiegoAndai moved this to Selected in Material UI Feb 23, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 25, 2024

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.

@oliviertassinari oliviertassinari removed the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Feb 25, 2024
@DiegoAndai
Copy link
Member Author

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
Copy link
Member

oliviertassinari commented Feb 27, 2024

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 DiegoAndai moved this from Selected to Backlog in Material UI Apr 22, 2024
@DiegoAndai
Copy link
Member Author

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?

@DiegoAndai DiegoAndai removed this from the Material UI: v6 milestone Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Collapse The React component deprecation New deprecation message package: material-ui Specific to @mui/material
Projects
Status: Backlog
Development

No branches or pull requests

2 participants