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 component #19704

Closed
2 tasks done
fromi opened this issue Feb 14, 2020 · 7 comments · Fixed by #26135
Closed
2 tasks done

[Hidden] Remove component #19704

fromi opened this issue Feb 14, 2020 · 7 comments · Fixed by #26135
Labels
Milestone

Comments

@fromi
Copy link

fromi commented Feb 14, 2020

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The Hidden component documentation states: "Any other props supplied will be provided to the root element (native element)." (see https://material-ui.com/api/hidden/)
However, the typescript declaration file for the component does not declare all the native underlying div properties, and the properties are not passed down, except for the className (which is not declared anyway for typescript users).
I use the CssHidden implementation of the component.

Expected Behavior 🤔

I should be able to add custom styles to the underlying div element using className or style properties.

Steps to Reproduce 🕹

Very simple: create a typescript project, use <Hidden mdUp implementation="css" className="whatever">, it does not compile.

Context 🔦

I want to add custom style to the div element generated by the Hidden component, using either className or style property (I need height="100%" at each level here).

I made this workaround for now:

const FixHidden = Hidden as React.ComponentType<HiddenProps & {className: string}>
@mbrookes mbrookes added the component: Hidden The React component. label Feb 14, 2020
@eps1lon eps1lon added docs Improvements or additions to the documentation and removed docs Improvements or additions to the documentation labels Mar 12, 2020
@eps1lon
Copy link
Member

eps1lon commented Mar 12, 2020

I don't have the full history for this component. For HiddenCSS we could spread the props to the div. However, the types need to reflect that only <Hidden implementation="css" /> supports this. <Hidden implementation="js" /> does not support this.

@fromi
Copy link
Author

fromi commented Mar 12, 2020

That's right, but it can be covered in the documentation isn't it ? It would not be the first component to expose properties without effect depending on another properties' value.

@eps1lon
Copy link
Member

eps1lon commented Mar 13, 2020

That's right, but it can be covered in the documentation isn't it ?

Docs should have a warning as well, yes.

Though it is technically correct. Props are spread to the root element in both cases. One case just doesn't have on ;)

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Mar 20, 2020
@oliviertassinari oliviertassinari removed the docs Improvements or additions to the documentation label Apr 10, 2020
@oliviertassinari
Copy link
Member

@fromi Thank you for opening this issue! I think that it's a great opportunity to take a step back and to reconsider what problem we wish this component to solve and how it fits into the global picture.

I have been wondering about removing this component in v5. Basically, developers could migrate way from this component.

  1. Replace HiddenCss with the system. For instance:
<Hidden mdDown implementation="css"><A /></Hidden>

would turn into:

<Box display={{ xs: 'none', lg: 'block' }} /><A /></Box>

This is documented in https://material-ui.com/system/display/#hiding-elements and comes from prior-art in:

  1. Replace HiddenJs with useMediaQuery. For instance:
<Hidden lgUp implementation="js"><A /></Hidden>

would turn into:

const hidden = useMediaQuery(theme => theme.breakpoints.up('lg')))

return hidden ? null : <A />;

So in this context, I think that we could keep pushing forward in a direction that gets ride of Hidden. Meaning, a deprecation in v4 and removal in v5. What do you guys think?

@fromi
Copy link
Author

fromi commented Apr 10, 2020

I think it would cover every use cases. I was using HiddenCss to avoid a blinking issue with server side rendering, but the "Box display" way should do the same.

@oliviertassinari oliviertassinari changed the title Hidden core component: props are not passed down nor declared Remove Hidden components Apr 13, 2020
@oliviertassinari oliviertassinari changed the title Remove Hidden components Remove Hidden component Apr 13, 2020
@oliviertassinari oliviertassinari added this to the v5 milestone Apr 13, 2020
@oliviertassinari oliviertassinari changed the title Remove Hidden component [Hidden] Remove component Aug 22, 2020
@0x636174
Copy link

for what it's worth, i vote to remove this 🤷‍♀️

@kongweiying2

This comment has been minimized.

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