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

[Grid] Fixing iOS Safari negative margins #23956

Closed
wants to merge 2 commits into from
Closed

[Grid] Fixing iOS Safari negative margins #23956

wants to merge 2 commits into from

Conversation

YuichiOnodera
Copy link

@YuichiOnodera YuichiOnodera commented Dec 11, 2020

Close #17142

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 11, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 067e324

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

@eps1lon
Copy link
Member

eps1lon commented Dec 11, 2020

Looks like this causes visual regressions: https://www.argos-ci.com/mui-org/material-ui/builds/6090

Was this supposed to be a breaking change?

@oliviertassinari oliviertassinari added the component: Grid The React component. label Dec 12, 2020
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.

The visual regression changes seem specific to the isolated env we use to take the screenshot. I couldn't spot changes in the documentation. Happy to give it a try. It would be great to have a second 👀 in the review.

@eps1lon
Copy link
Member

eps1lon commented Dec 12, 2020

But let's figure this out before we merge. Just because it looks ok for you, doesn't mean the change is safe.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 12, 2020

@eps1lon This would be great to double-check.

@eps1lon
Copy link
Member

eps1lon commented Dec 12, 2020

But let's figure this out before we merge. Just because it looks ok for you, doesn't mean the change is safe.

I haven't looked at it specifically but it sounds like that the change is not detectable in the docs due to

We apply box-sizing: content-box; in the visual regression tests and box-sizing: border-box; in the documentation to spot the few lines of CSS that aren't invariant.

-- #23934 (comment)

So this is might be a breaking change if you're using box-sizing: content-box. Considering the Grid is such a fundamental part of Material-UI and responsible for the layout we should reconsider this fix. As far as I know we don't document that we only support certain box-sizing models.

@oliviertassinari
Copy link
Member

So this is might be a breaking change if you're using box-sizing: content-box.

@eps1lon Oh smart, I didn't consider this option. it can be tested in the docs by changing the box-sizing back & forth.

Capture d’écran 2020-12-12 à 19 53 03

I see no changes on my end but please double check :).

@YuichiOnodera
Copy link
Author

Not sure, but looks like a browser bug (Chrome, Safari). When we use margin: -32px; on a container, the width of the container is reduced by 64px, and that's okay. But when we change to margin: calc(0% - 32px), the width of the container doesn't shrink.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 14, 2020

@YuichiOnodera Yeah, you are right. I move one of the visual screenshots to codesandbox:

Capture d’écran 2020-12-14 à 11 59 52

Capture d’écran 2020-12-14 à 12 00 01

Developers would no longer get the correct width & placement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Grid The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Grid] iOS & Safari negative margins issue
4 participants