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

[Stack, system] Apply correct responsive styles if any custom breakpoints are provided #32913

Merged

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented May 26, 2022

Fixes #32214


First of all, a very intriguing bug.

See the following:

String property access

Particularly interesting, since the issue reporter was having a small custom breakpoint value and for other breakpoints properties it was not causing any issue.

And if the input of direction or spacing is a string in Stack (here it's direction="column"), this particular line will evaluate to true due to it being a function as shown in the above image.

The String object has small method in it's prototype. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/small.

It's wrong to access the property if the input is a string. So first we check that it's of type object, then only we create a custom base.
Same applies to resolveBreakpointValues function. So, it's also a regression from #29300.

Before: https://codesandbox.io/s/peaceful-https-6iewi9?file=/src/Demo.tsx
Now: https://codesandbox.io/s/jolly-volhard-247y2y?file=/src/Demo.tsx

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work package: system Specific to @mui/system component: Stack The React component. regression A bug, but worse labels May 26, 2022
@mui-bot
Copy link

mui-bot commented May 26, 2022

Details of bundle changes

Generated by 🚫 dangerJS against a46cd9f

@ZeeshanTamboli ZeeshanTamboli changed the title [Stack, system] Apply correct responsive styles with custom breakpoints [Stack, system] Apply correct responsive styles if any custom breakpoints May 26, 2022
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review May 26, 2022 05:37
@ZeeshanTamboli ZeeshanTamboli changed the title [Stack, system] Apply correct responsive styles if any custom breakpoints [Stack, system] Apply correct responsive styles if any custom breakpoints are provided May 26, 2022
@ZeeshanTamboli
Copy link
Member Author

Also, not complaining, but just wanted to say that TypeScript would have prevented this type of issue. 😁

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

This is what I had in mind, sorry if it was not clear.

packages/mui-material/src/Stack/Stack.test.js Outdated Show resolved Hide resolved
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Logic looks good 👍

@mnajdova mnajdova merged commit e55bafd into mui:master Jun 8, 2022
@ZeeshanTamboli ZeeshanTamboli deleted the issue/32214-custom-breakpoint-named-small branch June 8, 2022 12:52
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: Stack The React component. package: system Specific to @mui/system regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[theme] Getting styleFromPropValue for spacing will return undefined when using custom breakpoint named small
3 participants