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] Improve breakpoints resolver function #29300

Merged
merged 12 commits into from
Oct 28, 2021

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Oct 26, 2021

Closes #26773
Closes #29175

Bugs prior to this PR:

  • [Grid] Unordered responsive prop values for spacing, direction, rowSpacing, columnSpacing, are not resolved correctly; e.g., direction={{sm: 'row', xs: 'column'}} stays as column` in all window sizes.
  • [Grid] Array type of responsive prop values are not resolved correctly; e.g., columns={{ xs: 4, sm: 8, md: 12 }} and columns=[4,8,12] behave differently.
  • [Stack] Unordered responsive prop values for spacing and direction, are not resolved correctly; e.g., direction={{sm: 'row', xs: 'column'}} stays as column` in all window sizes.
  • code sandbox with comments: https://codesandbox.io/s/naughty-brahmagupta-i0f24?file=/src/Demo.tsx:0-2065

After this PR:

@mui-pr-bot
Copy link

mui-pr-bot commented Oct 26, 2021

Details of bundle changes

@material-ui/core: parsed: +0.10% , gzip: +0.11%
@material-ui/lab: parsed: +0.12% , gzip: +0.08%

Generated by 🚫 dangerJS against 9ff4938

@hbjORbj hbjORbj added component: Grid The React component. component: masonry This is the name of the generic UI component, not the React module! component: Stack The React component. bug 🐛 Something doesn't work package: system Specific to @mui/system labels Oct 26, 2021
@hbjORbj hbjORbj changed the title [WIP] [system] Improve breakpoints resolver function [system] Improve breakpoints resolver function Oct 26, 2021
@hbjORbj hbjORbj requested review from siriwatknp, mnajdova, michaldudak and eps1lon and removed request for siriwatknp October 26, 2021 09:46
@eps1lon eps1lon removed their request for review October 26, 2021 10:39
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.

Looks good let's just add tests for resolveBreakpointValues to make sure we don't break it in the future

// compute base for responsive values; e.g.,
// [1,2,3] => {xs: true, sm: true, md: true}
// {xs: 1, sm: 2, md: 3} => {xs: true, sm: true, md: true}
function computeBreakpointsBase(breakpointValues, themeBreakpoints) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice that we have this extracted :)

packages/mui-system/src/breakpoints.js Show resolved Hide resolved
@hbjORbj hbjORbj requested a review from mnajdova October 27, 2021 12:46
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.

From looking at the tests it looks good.

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.

👍 Niceee!

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. component: masonry This is the name of the generic UI component, not the React module! component: Stack The React component. package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Breakpoints order on grid direction shouldn't affect the behaviour
4 participants