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] Fix Grid v1 spacing when using CSS variables #44663

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Dec 5, 2024

Summary

Closes: #44662

Sadly, we went back and forth in regressions with #40224, #42574 and #44376.

The issue

This latest regression is caused because -calc(...) values are not valid, see https://codepen.io/diegoandai/pen/qEWboqp. The Grid is outputting -calc(...) margin values when cssVariables: true:

Screenshot 2024-12-05 at 16 47 45

The solution

This PR fixes it by replacing-calc(...) with calc(-1 * calc(...)). It also replaces -<pixelValue>px with calc(-1 * pixelValue) as I don't think it's worth it adding the logic to differentiate between cases inside the Grid.

Before: https://stackblitz.com/edit/react-ziw5rb?file=Demo.tsx,package.json
After: https://codesandbox.io/p/sandbox/pull-44663-after-jvt2tj

Post mortem

As a post-mortem of the multiple regressions, what I think enabled them was not having enough tests with each PR to cover the cases being fixed.

What I don't like about this fix is that it also changes the implementation for users who are not using CSS variables. Although the result should be the same, I hope there are no edge cases to my approach. If there are an ideas for how to fix this without changing the functionality for non-CSS variables users, I'll be happy to refactor the solution.

@DiegoAndai DiegoAndai added component: Grid The React component. package: material-ui Specific to @mui/material regression A bug, but worse labels Dec 5, 2024
@DiegoAndai DiegoAndai requested a review from siriwatknp December 5, 2024 19:49
@DiegoAndai DiegoAndai self-assigned this Dec 5, 2024
@mui-bot
Copy link

mui-bot commented Dec 5, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against d434d75

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

This makes the most sense. Delate the calculation to CSS if we can 👍, I like it.

@DiegoAndai DiegoAndai merged commit 76f5438 into mui:master Dec 9, 2024
19 checks passed
@DiegoAndai DiegoAndai deleted the fix-css-variables-grid-spacing branch December 9, 2024 20:20
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Dec 9, 2024
@oliviertassinari oliviertassinari changed the title [material-ui][Grid] Fix spacing when using css variables [Grid] Fix spacing when using css variables Dec 10, 2024
@oliviertassinari oliviertassinari changed the title [Grid] Fix spacing when using css variables [Grid] Fix Grid v1 spacing when using css variables Dec 10, 2024
@oliviertassinari oliviertassinari changed the title [Grid] Fix Grid v1 spacing when using css variables [Grid] Fix Grid v1 spacing when using CSS variables Dec 10, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 10, 2024

Sadly, we went back and forth in regressions

Lol.

1 #40224 breaks Grid v1 with CSS variables
2 #42574 fixes it, adds test for other stuff in #40224 that felt missing, but doesn't add test for the fix
3 #44376 fixes the regression introduced by 2. but reintroduces the bug 2 aimed to fix, proof https://deploy-preview-44376--material-ui.netlify.app/base-ui/, back to 1
4 #44663 fixes 1, add tests that should have been added in 2.

I think it's why having clear scopes with clear https://www.notion.so/mui-org/Product-owners-836c72feffcc4cb2b735b51527e6991a?pvs=4#01db303c540643969c7274ef6c823ba5 is important. Tests help for sure scale the ownership, but they don't allow to be as fast and do as quality work with 3-4 people iterating on the logic, as 1 can.

What I don't like about this fix is that it also changes the implementation for users who are not using CSS variables. Although the result should be the same, I hope there are no edge cases to my approach. If there are an ideas for how to fix this without changing the functionality for non-CSS variables users, I'll be happy to refactor the solution.

We could solve this with a partial revert of https://github.com/mui/material-ui/pull/42574/files#r1632084146. This wasn't truly accurate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Grid The React component. package: material-ui Specific to @mui/material regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Grid] Spacing doesn't work when using css variables.
4 participants