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.

Sign up for GitHub

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

Jump to bottom

[Grid] Fix regression v1 spacing prop with string value #44376

Merged
merged 2 commits into from
Nov 13, 2024
Merged

[Grid] Fix regression v1 spacing prop with string value #44376

merged 2 commits into from
Nov 13, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Nov 11, 2024 •
edited
Loading

closes #44321

Regression from #42574, test added.

@siriwatknp siriwatknp added component: Grid The React component. regression A bug, but worse labels Nov 11, 2024
@mui-bot
Copy link

mui-bot commented Nov 11, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-44376--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 90a91d1

DiegoAndai
DiegoAndai reviewed Nov 11, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Solution looks good 👍🏼 just have a question regarding the test

packages/mui-material/src/Grid/Grid.test.js Outdated
@@ -746,7 +746,7 @@ describe('Material UI <Grid />', () => {
generateRowGap({
ownerState: {
container: true,
rowSpacing: { xs: 1, sm: 2 },
rowSpacing: { xs: 1, sm: '16px' },
Copy link
Member

@DiegoAndai DiegoAndai Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we keep the sm: 2 test as well instead of replacing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added md: '16px' instead.

@siriwatknp siriwatknp mentioned this pull request Nov 12, 2024
Merged
DiegoAndai
DiegoAndai approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@siriwatknp siriwatknp merged commit 981b033 into mui:master Nov 13, 2024
19 checks passed
@rroslaniec
Copy link

rroslaniec commented Nov 28, 2024 •
edited
Loading

I think this PR broke one thing on my website.

When you are combining Container with Grid then the margin-top and margin-left is not calculated properly for the Grid or maybe for the Container - hard to say; but this change definitely caused that.

Look

before Zrzut ekranu 2024-11-28 o 17 08 17 Zrzut ekranu 2024-11-28 o 17 08 25
after Zrzut ekranu 2024-11-28 o 17 06 50 Zrzut ekranu 2024-11-28 o 17 07 09

The difference is on Grid element that margin top and left styles changed:
from:

    margin-top: calc(-2* var(--mui-spacing));
    margin-left: calc(-2* var(--mui-spacing));

to:

    margin-top: -calc(2* var(--mui-spacing));
    margin-left: -calc(2* var(--mui-spacing));

Worth to mention that before variable era everything was alright with the styles

cc @siriwatknp @DiegoAndai

@DiegoAndai
Copy link
Member

Thanks for the report @rroslaniec, I can confirm this is a bug, I'll open an issue to track it.

This was referenced Dec 5, 2024
Closed
Merged
@DiegoAndai
Copy link
Member

@rroslaniec the PR to solve your issue was merged: #44663
It will be available from the next release (>6.1.10)

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Dec 10, 2024
@oliviertassinari oliviertassinari changed the title [material-ui][Grid v1] Fix regression spacing prop with string value [Grid] Fix regression v1 spacing prop with string value Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiegoAndai DiegoAndai DiegoAndai approved these changes

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: Grid The React component. regression A bug, but worse
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

In v6, Grid columnSpacing misbehaves with string pixel values like "16px"
5 participants
@siriwatknp @mui-bot @rroslaniec @DiegoAndai @oliviertassinari