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 #26135

Merged
merged 2 commits into from
May 6, 2021
Merged

[Hidden] Remove component #26135

merged 2 commits into from
May 6, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented May 4, 2021

Breaking changes

  • [Hidden] Removed in favor of using the sx prop or the useMediaQuery hook.

    Use the sx prop to replace implementation="css":

    -<Hidden implementation="css" xlUp><Paper /></Hidden>
    -<Hidden implementation="css" xlUp><button /></Hidden>
    +<Paper sx={{ display: { xl: 'none', xs: 'block' } }} />
    +<Box component="button" sx={{ display: { xl: 'none', xs: 'block' } }} />

    Use the useMediaQuery hook to replace implementation="js":

    -<Hidden implementation="js" xlUp><Paper /></Hidden>
    +const hidden = useMediaQuery(theme => theme.breakpoints.up('xl'));
    +return hidden ? null : <Paper />;

Part of #20012
Closes #19704
Preview: https://deploy-preview-26135--material-ui.netlify.app/

@mui-pr-bot
Copy link

mui-pr-bot commented May 4, 2021

Details of bundle changes

@material-ui/core: parsed: -0.51% 😍, gzip: -0.63% 😍

Generated by 🚫 dangerJS against 192bd5f

This was referenced May 5, 2021
paper: classes.paper,
}}
variant="permanent"
sx={{ display: { xs: 'none', lg: 'block' } }}
Copy link
Member

Choose a reason for hiding this comment

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

question: Assuming we just want to migrate from <Hidden lgDown />. Do we need lg: 'block' here? To me it isn't clear what the display is above lg or why we even need to differentiate between above and below lg.

Documentation for this is lacking so whatever is the takeway, I'll incorporate into the docs.

Copy link
Member

Choose a reason for hiding this comment

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

So I eventually found out how these works from the docs and it seems to me that { xs: 'none', lg: 'block' } is not equivalent to lgDown if the screen width is below xs.
Wouldn't { display: 'none', lg: { display: 'none' }} actually be equivalent?
Though maybe lg isn't recognized and we would need { [theme.breakpoints.down('lg')]: { display: 'none' }.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting feedback, xs is meant to be width: 0px but it's not very clear. I wonder how we could fix this. I have seen some name it differently, like base.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for

{ [theme.breakpoints.down('lg')]: { display: 'none' } }

it's much more clear. Otherwise, I personally have hard time to follow what will be the end result.

Copy link
Member

Choose a reason for hiding this comment

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

@mnajdova Then I guess it's a matter of habit and we should seriously consider #25846.

I'm biased, I'm used to thinking "mobile-first", hence why sx={{ display: { xs: 'none', lg: 'block' } } reads naturally to me.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe, my point was more, if I see lgDown, I would be able to immediately replace it with:

{ [theme.breakpoints.down('lg')]: { display: 'none' } }

without thinking about it... 🤷 Otherwise, it would require some mental exercise 😄

Copy link
Member Author

@m4theushw m4theushw May 5, 2021

Choose a reason for hiding this comment

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

Why not add support for props like xxDown and xxUp in the sx prop? It would translate nicely with the props of Hidden.

Interesting feedback, xs is meant to be width: 0px but it's not very clear. I wonder how we could fix this. I have seen some name it differently, like base.

base would be great.

I'm biased, I'm used to thinking "mobile-first", hence why sx={{ display: { xs: 'none', lg: 'block' } } reads naturally to me.

For me, it's not natural. We have a dilemma between users that think mobile-first and desktop-first.

{ [theme.breakpoints.down('lg')]: { display: 'none' } }

I changing to this as I think it's more readable. However, I can't use it outside a component, like here.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, not a blocker, I think it's personal preference, I wouldn't change the code to be honest. @eps1lon can we resolve?

Copy link
Member

Choose a reason for hiding this comment

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

Migration is important. Please don't just handwave these things. Existing users and their path forward is critical to trust.

This is a general frustration I have where we constantly add new features completely ignoring that a feature has inherent cost associated with it. But as soon as we discuss interactions with other features, the feature is discarded.

People can customize breakpoints. The current migration path breaks in those customization scenarios. So whatever we do suggest as a migration either needs to actually be equivalent. Or justify why we ignore custom breakpoints. Which would be odd considering we just improved them with module augmentation.

Copy link
Member

Choose a reason for hiding this comment

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

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.

If we feel we are removing the component prematurely, we could only deprecate it, and dive deeper in the migration for v6.

paper: classes.paper,
}}
variant="permanent"
sx={{ display: { xs: 'none', lg: 'block' } }}
Copy link
Member

Choose a reason for hiding this comment

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

@ocodista
Copy link

ocodista commented May 17, 2021

How to replace <Hidden mdDown>? This change breaked my project

@oliviertassinari
Copy link
Member

How to replace ?

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

@mnajdova
Copy link
Member

mnajdova commented May 17, 2021

Maybe we should update the changelog to show this example as well, or at least the migration guide.

@valentyna-antoniuk
Copy link

It looks like everyone who wants to migrate to the new version will have to create their own version of this component, otherwise they will rewrite the hiding of elements throughout the project, which entails a lot of edits, even in my not very large project.

Is this the final decision to remove Hidden component and not deprecate it? Thanks.

@oliviertassinari
Copy link
Member

@missi2402 It's coming back as deprecated in #26408

@valentyna-antoniuk
Copy link

@oliviertassinari Thanks for the link!

siriwatknp added a commit to siriwatknp/material-ui that referenced this pull request Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Hidden] Remove component
7 participants