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

[system] Fix toolbar media query mixin getting merged in wrong order #32713

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented May 10, 2022

Solution was proposed in #27532 (comment)

The styles were getting injected in the wrong order.

Before:
Wrong order

After:
Correct order

Notice the media query styles of the Box component. However, I couldn't find any reason as to why it is happening after a lot of debugging. I think it's something to do with the styleFunctionSx function because the mixins works correctly with the styled API and also with raw media queries. Other reason is that mixins work in the Toolbar component because it is wrapped with the styled API.

Any customized mixins defined in theme are just merged in the createMixins method. So those won't get affected.

Regarding tests, I couldn't test responsive styles by resizing the window in karma browser testing for both Toolbar and with Box. I think we need an e2e test. Let me know how.

Verified manually that it works:
Before: https://codesandbox.io/s/mystifying-hoover-yf2ehh?file=/src/App.js
Now: https://codesandbox.io/s/polished-pond-hq4vkr?file=/src/App.js


Fixes #31358
Fixes #27532


Default theme object changed for mixins.toolbar (Preview: https://deploy-preview-32713--material-ui.netlify.app/material-ui/customization/default-theme/)

@mui-bot
Copy link

mui-bot commented May 10, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 8e586a8

@ZeeshanTamboli ZeeshanTamboli changed the title [system] Fix toolbar media query mixins getting merged in wrong order [system] Fix toolbar media query mixin getting merged in wrong order May 10, 2022
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review May 10, 2022 10:26
@ZeeshanTamboli ZeeshanTamboli added package: system Specific to @mui/system bug 🐛 Something doesn't work labels May 10, 2022
@mnajdova
Copy link
Member

The problem is with our logic for mering breakpoints. Take a look at https://github.com/mui/material-ui/blob/master/packages/mui-system/src/styleFunctionSx/styleFunctionSx.js#L61 (we are creating an empty object containing all breakpoints in the correct order, and then just add values here). The problem is that this breakpoint here contains the and so it is not the same string as the breakpoint media query. This is not an ideal implementation, but it's the best we can do (at least in my opinion). The solution looks good, I would just ensure that it works by adding test case for it.

@ZeeshanTamboli
Copy link
Member Author

The problem is with our logic for mering breakpoints. Take a look at https://github.com/mui/material-ui/blob/master/packages/mui-system/src/styleFunctionSx/styleFunctionSx.js#L61 (we are creating an empty object containing all breakpoints in the correct order, and then just add values here). The problem is that this breakpoint here contains the and so it is not the same string as the breakpoint media query. This is not an ideal implementation, but it's the best we can do (at least in my opinion). The solution looks good, I would just ensure that it works by adding test case for it.

Yes thanks, I got it now. It is not merging the toolbar mixins values properly with the empty breakpoints when the and keyword is present in breakpoint.

I also added a test for it.

@mnajdova mnajdova merged commit a690f4d into mui:master May 18, 2022
@ZeeshanTamboli ZeeshanTamboli deleted the fix-toolbar-mixin-is-merging-in-wrong-order-using-system branch May 18, 2022 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: system Specific to @mui/system
Projects
None yet
3 participants