When customizing breakpoints with createMuiTheme, only a shallow merged value is created

[escapiststupor]

When customizing breakpoints with createMuiTheme, only a shallow merged value is created

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

When providing a customized theme object, createMuiTheme should deep merge it.

Current Behavior 😯

createMuiTheme only shallow merges breakpoints.value.

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/jovial-thunder-hgw6u

Context 🔦

I am sure this is not new in v4 - not sure if this is the intended behaviour, however, when I tried to custom breakpoints by providing a customized theme object to createMuiTheme like this:

const theme = createMuiTheme({
  breakpoints: {
    values: {
      lg: 1500,
      xl: 1300
    }
  }
});

I thought createMuiTheme will deep merge the customized theme object with the default values so I thought for the rest of the keys not provided it will fallback to use the default values of:

{
  xs: 0,
  sm: 600,
  md: 960
}

However I bumped into some bugs and after digging into the source code, I found out that when createMuiTheme calls createBreakpoints it overrides the value key inside breakpoints completely without merging the respective values.

Therefore, when the display size falls in an undefined value, the withWidth HOC returns some invalid value as well, thus breaking some features relying on withWidth such as <Hidden />.

Is a deep merge for createBreakpoints not implemented due to performance issues? If so, maybe it will be a good idea to document it better, instead of saying

because

Takes an incomplete theme object and adds the missing parts.

sounds like a deep merge of all keys to me. On top of that the implementation details inside createMuiTheme indeed deepmerge some keys, which makes it more confusing.

Your Environment 🌎

Tech	Version
Material-UI	v4.1.3
React	v16.8.6

[eps1lon]

Your console logged warnings that the classes prop was missing. You never wrapped your Navigator in withStyles.

Apart from that I see no usage of breakpoints in your codesandbox. Inspecting the tree with the experimental react dev tools reveals they are correctly merged into the material-ui theme:

[escapiststupor]

@eps1lon Sorry my bad, that wasn't the most updated version of codesandbox. I have updated it already; and as you can see from the devtool, the values object only contains one key (xl).

I believe the problem isn't about the use of withStyles or not. It's about the <Hidden /> component - as you can see in the updated codesandbox, the Hidden element with mdUp prop is always hidden.

However, if you remove the theme provider and use the default theme, it works.

[eps1lon]

Now I understand. You want the breakpoints to be merged instead of replaced.

I think we should come up with a better strategy. If we implement your expected behavior it's easy to pass in non-sensical breakpoints where a semantically larger breakpoint is numericaly smaller e.g. xl: 200.

So I'm not sure if this is a docs issue or if we should support it. Until then I'd recommend using

const customStyle = {
  breakpoints: {
    values: {
+      xs: 0,
+      sm: 600,
+      md: 960,
+      lg: 1280,
       xl: 1300,
    }
  }
};

[escapiststupor]

Yes, you do make sense. If it will be supported in the future, maybe throw an error/warning when the numbers don't make sense? For now maybe it will be better to document this in more detail to avoid naive users like me.

[oliviertassinari]

I think we should come up with a better strategy.

@eps1lon I was hit by this problem at Onepixel 8 months ago. I agree, we should and we can do better. I might require a BC.

[eps1lon]

But honestly looking at this now I'd rather have a warning that there are missing breakpoints over merging breakpoints. Every time you look at this code you also have to know default values. Yes this is opinionated but it's really not that hard to copy 4 values.

[oliviertassinari]

There is an opportunity for a warning too 👍