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

[Hidden] Remove deprecated API #44073

Closed
oliviertassinari opened this issue Oct 11, 2024 · 5 comments · Fixed by #45283
Closed

[Hidden] Remove deprecated API #44073

oliviertassinari opened this issue Oct 11, 2024 · 5 comments · Fixed by #45283
Assignees
Labels

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 11, 2024

Steps to reproduce

  1. Open https://mui.com/material-ui/react-hidden/

Context

#42305. This component was deprecated in v5.0.0, so on September 16, 2021. The migration path is documented in https://mui.com/material-ui/migration/v5-component-changes/#replace-deprecated-component.

Added to the v7 milestone per #42305 (comment).

Your environment

v6.1.3

@oliviertassinari
Copy link
Member Author

Let's pause this until we discuss if we want to remove Hidden or revisit its API. Hidden keeps things composable and doesn't force the use of the sx prop for devs not directly using Emotion or Pigment CSS. source

@aarongarciah Tailwind CSS users have their own utils, so I imagine it's really only about CSS Modules users.

Considering that sx is always available, I don't know. It seems that we can't get composition without using an extra DOM node or using React.cloneElement() which both feel: nop?

@aarongarciah
Copy link
Member

Considering that sx is always available

@oliviertassinari what do you mean by this? I'm assuming you mean sx is always available because we "force" consumers to use our styling solution (Emotion/styled-components/Pigment CSS as of today).

It seems that we can't get composition without using an extra DOM node or using React.cloneElement() which both feel: nop?

I also don't like using either of those two options, and I think Hidden is a weird API, but if we end up pursuing a future in which we don't force consumers to use a styling solution (i.e. offer static CSS files consumers can load), how are you going to offer consumers a way to hide/show stuff that supports responsive values? Our current model for building layouts is components, so although I don't like using an extra dom element or React.cloneElement() 1, maybe is the least bad option if we don't want to change our model for building layouts.

To be honest, my preferred API for building layouts these days would be to have a set of typed helper functions that just return class names (see https://x.com/devongovett/status/1836765141204885694). But that's a longer discussions.

tl;dr: I'm fine with removing Hidden, but let's dig into this topic more becase there are a lot of interrelated things we need to take into account.

Footnotes

  1. Base UI probably already makes heavy use of React.cloneElement, so not sure that's a real problem? Honest question.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 14, 2024

I'm assuming you mean sx is always available because we "force" consumers to use our styling solution (Emotion/styled-components/Pigment CSS as of today).

@aarongarciah I assumed that if people use one of the existing components, we force them one of the styling solutions that we support. Today, all support sx.

At least, with JSS from v1 to v4, we never really felt forcing JSS was as a big problem. People were happy to use JSS and customize the component with something else. It didn't seem to truly matter because we designed it to play nicely with CSS modules, emotion, styled components, etc. However, it matters if JSS fails to deliver like emotion does in today's world.

So IMHO, if we could get to one great versatile styling solution, it could be good enough. In the code ejection story, it could be a bit different, maybe we need native Tailwind CSS too, but I don't know, I saw a lot of people say they hate that Shadcn UI uses Tailwind CSS but still uses Shadcn UI anyway because of the rest of the value.

if we end up pursuing a future in which we don't force consumers to use a styling solution (i.e. offer static CSS files consumers can load),

Right, yeah, I don't think Pigment CSS, in its current state with some of the bundlers supports, can replace emotion in Material UI, so it can't truly support it as JSS did. Pigment CSS either needs to have a. .css output mode, b. ALL bundlers supported (sounds not possible), or c. a smart runtime (a successor to emotion) or d. Material UI to support other styling engines.

how are you going to offer consumers a way to hide/show stuff that supports responsive values?

  1. We could leave this up to the user. Propose them to use something CSS variables based, e.g. https://open-props.style/ API.
  2. We could do 1. but built-in.
  3. We could leave this up to the user. Propose them to use Tailwind CSS.
  4. We could do 3. but built-in.
  5. We have them use or styling engine that supports sx
  6. We generate a few class names ahead of time: [Drawer] New improvements #7925. Not very versatile but works.
  7. I'm out of idea 😄?

I don't like using an extra dom element or React.cloneElement() 1, maybe is the least bad option if we don't want to change our model for building layouts.

True, the least bad option reasoning is a valid way to look at it.

I'm fine with removing Hidden, but let's dig into this topic more becase there are a lot of interrelated things we need to take into account.

I personally don't mind either direction. The origin of this component: #6726 (comment).

@DiegoAndai DiegoAndai moved this to Backlog in Material UI Jan 22, 2025
@DiegoAndai DiegoAndai self-assigned this Jan 22, 2025
@aarongarciah
Copy link
Member

After careful consideration, I think removing the Hidden component is the way to go. Requiring a DOM node in order to hide something is not good, plus Material UI chose the sx prop over Box/system props.

cc @DiegoAndai

@DiegoAndai DiegoAndai moved this from Backlog to In progress in Material UI Feb 7, 2025
@github-project-automation github-project-automation bot moved this from In progress to Done in Material UI Feb 12, 2025
Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants