[theme] Remove hardcoded breakpoints

[nkrivous]

There are several places in the codebase where breakpoints are hardcoded.
Since we have the ability to override breakpoints, we should not have hardcoded values in the code

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

1. PropTypes

PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl']),
PropTypes.arrayOf(PropTypes.oneOf(['xs', 'sm', 'md', 'lg', 'xl'])),

2. Styles

[theme.breakpoints.only('xs')]: {
  ...
}

3. Functions in withWidth.js

import { breakpointKeys } from '../styles/createBreakpoints';
function isWidthUp(...){ 
  return breakpointKeys.indexOf(...)
}
function isWidthDown(...){ 
  return breakpointKeys.indexOf(...)
}

Expected Behavior 🤔

1. PropTypes

PropTypes.string
PropTypes.arrayOf(PropTypes.string),

2. Styles

a) Use a media query only if there is such a breakpoint in the theme

[theme.breakpoints.only(theme.breakpoints.keys['xs'] ?? 0)]: {
  ...
}

b) Add a new property alias to createBreakpoints function to match breakpoints

createBreakpoints({
  values: {
      tablet: 640,
      laptop: 1024,
      desktop: 1280,
    },
    alias: {
      xs: 'tablet',
      sm: 'tablet',
    },
  }
})

3. Functions in withWidth.js
   a) Show the necessity for breakpoints
   Remove import { breakpointKeys } from '../styles/createBreakpoints';

function isWidthUp(breakpoints, breakpoint, width, inclusive = true){ .. }

b) Add breakpoints to options implicitly

import { breakpointKeys } from '../styles/createBreakpoints'
function isWidthUp(breakpoint, width, opts = {}) {
  let {breakpoints = breakpointKeys, inclusive = true } = opts;
  ...
}

Your Environment 🌎

Tech	Version
Material-UI	v4.11.0

[oliviertassinari]

• Did you look at how Bootstrap solve the problem?
• Have you considered implementing the alias behavior by overriding the helper? If it's good enough, we can update the documentation for it. I like the tradeoff.
• Is TypeScript typing enough to ensure valid breakpoint keys are used?

[oliviertassinari]

The two possible solutions I can think of:

diff --git a/docs/src/pages/customization/breakpoints/breakpoints.md b/docs/src/pages/customization/breakpoints/breakpoints.md
index 0f5ec2fe63..6a8b9402db 100644
--- a/docs/src/pages/customization/breakpoints/breakpoints.md
+++ b/docs/src/pages/customization/breakpoints/breakpoints.md
@@ -125,6 +125,23 @@ const theme = createMuiTheme({
     },
   },
 });
+
+const aliases = {
+  xs: 'tablet',
+  sm: 'tablet'
+  md: 'laptop',
+  lg: 'desktop',
+  xl: 'desktop',
+};
+
+const getAliasedKey = (key) => (aliases[key] ? aliases[key] : key);
+
+theme.breakpoints.up = (key) => theme.breakpoints.up(getAliasedKey(key));
+theme.breakpoints.down = (key) => theme.breakpoints.down(getAliasedKey(key));
+theme.breakpoints.between = (start, end) => (
+  theme.breakpoints.only(getAliasedKey(start), getAliasedKey(end))
+);
+theme.breakpoints.only = (key) => theme.breakpoints.only(getAliasedKey(key));
 ```

 If you are using TypeScript, you would also need to use [module augmentation](/guides/typescript/#customization-of-theme) for the theme to accept the above values.

or

diff --git a/packages/material-ui/src/styles/createBreakpoints.js b/packages/material-ui/src/styles/createBreakpoints.js
index 0d7e133abe..dd6787eef7 100644
--- a/packages/material-ui/src/styles/createBreakpoints.js
+++ b/packages/material-ui/src/styles/createBreakpoints.js
@@ -14,19 +14,23 @@ export default function createBreakpoints(breakpoints) {
       lg: 1280,
       xl: 1920,
     },
+    aliases = {},
     unit = 'px',
     step = 5,
     ...other
   } = breakpoints;

+  const getAliasedKey = (key) => (aliases[key] ? aliases[key] : key);
   const keys = Object.keys(values);

   function up(key) {
+    key = getAliasedKey(key);
     const value = typeof values[key] === 'number' ? values[key] : key;
     return `@media (min-width:${value}${unit})`;
   }

   function down(key) {
+    key = getAliasedKey(key);
     const endIndex = keys.indexOf(key) + 1;
     const upperbound = values[keys[endIndex]];

@@ -40,6 +44,8 @@ export default function createBreakpoints(breakpoints) {
   }

   function between(start, end) {
+    start = getAliasedKey(start);
+    end = getAliasedKey(end);
     const endIndex = keys.indexOf(end);

     if (endIndex === keys.length - 1) {
@@ -60,6 +66,7 @@ export default function createBreakpoints(breakpoints) {
   }

   function only(key) {
+    key = getAliasedKey(key);
     return between(key, key);
   }

[nkrivous]

Did you look at how Bootstrap solve the problem?

Good point. Thank you. I will take a look at how their sass mixins work.

Have you considered implementing the alias behavior by overriding the helper? If it's good enough, we can update the documentation for it. I like the tradeoff.
I don't think that only adding an example to documentation.

A Tabs.js defines a CSS class

 scrollButtonsDesktop: {
    [theme.breakpoints.down('xs')]: {
      display: 'none',
    },
  },

on the issue under consideration

const aliases = {
  xs: 'tablet', //640
  sm: 'tablet', //640
  md: 'laptop', //1024
  lg: 'desktop', //1280
  xl: 'desktop', //1280
};

we get the string for .scrollButtonsDesktop:

@media (max-width:1023.95px) {
      display: 'none',
},

although we didn't want to hide the scroll buttons at all.

I think that in the mapping there should be only significant breakpoints:

const aliases = {
  sm: 'tablet', //640
  md: 'laptop', //1024
  lg: 'desktop', //1280
};

and for theme.breakpoints.down('xs') it should not generate any output (or can give invalid output @media {...})

Is TypeScript typing enough to ensure valid breakpoint keys are used?

Yes, it is.

export interface Breakpoints {
  ...
  aliases: Partial<{[key in keyof BreakpointDefaults]: keyof BreakpointValues}>;
}

What do you think about changing the signatures of the functions isWidthUp and isWidthDown (point 3)?

[oliviertassinari]

we get the string for .scrollButtonsDesktop:

This is another issue: #13448 if you are interested.

I think that in the mapping there should be only significant breakpoints
it should not generate any output

Interesting, consideration. I think that if the key or alias isn't present, it should raise a console.error.
The choice should be explicit to the developers.

What do you think about changing the signatures of the functions isWidthUp and isWidthDown (point 3)?

We want to change these helpers, at the very least replace them with hooks or maybe even better drop it #17350.

[nkrivous]

This is another issue: #13448 if you are interested.

Hmm. Not exactly. I mean, if we have breakpoints starting only with tablet, therefore, we don’t care about the smaller size, so we don’t want to have a behavior for the size of xs.

Interesting, consideration. I think that if the key or alias isn't present, it should raise a console.error.
The choice should be explicit to the developers.

Another example, the material core introduces some component FooComponent, which is hidden on mobile devices but shown on other screen sizes.
The implementation may be as follows:

const styles = {
 root: {
 display: 'flex',
  [theme.breakpoints.down('xs')]: {
      display: 'none',
   },
};

if we define aliases for every breakpoint

const aliases = {
  xs: 'tablet', //640
  sm: 'tablet', //640
  ...
}

xs will be replaced by tablet and the style will be

const styles = {
 root: {
 display: 'flex',
  [theme.breakpoints.down('tablet')]: {
      display: 'none',
   },
};

which is incorrect, since FooComponent should be visible on screens larger than mobile.

Maybe explicit behavior means "I want to have code generated only for the specified breakpoints"?

[oliviertassinari]

These considerations should be handled with overrides, not the breakpoint helper.

[nkrivous]

I understood your idea that behavior with partially defined aliases is not clear at first sight.
On the other hand, fully defined aliases can lead to unexpected side effects that can be determined only if you dive into the source code, or, even worse in production.

There is another suggestion not to generate code for missing breakpoints and not to have aliases at all.

Did you look at how Bootstrap solve the problem?

Bootstrap does not work with components at a high level.
For example, it gives us the navbar class, which is not responsive, but also some other helper classes ( .navbar-expand-lg) that define responsive behavior:

<nav class="navbar navbar-expand-lg">
  <button class="navbar-toggler" />
  <div class="collapse navbar-collapse" /> 
</nav>

.collapse {
    display: none;
}

@media (min-width: 992px) {

  .navbar-expand-lg .navbar-collapse {
      display: flex;
  }

  .navbar-expand-lg .navbar-toggler {
      display: none;
  }
}

Of course, if we override breakpoints

$grid-breakpoints: (
  tablet: 640px,
  laptop: 1024px,
  desktop: 1280px,
);

the example above will not work, because another class will be generated (.navbar-expand-desktop). To fix this, we must use our own overridden classes.

If Material-UI had the same Nav component, it would continue to generate CSS Media for navbar-expand-lg, which is not used in the new breakpoint map.

[oliviertassinari]

There is another suggestion not to generate code for missing breakpoints and not to have aliases at all.

@nkrivous I don't think that this can work because we wouldn't know what to do with the media query, it can be a up as much as a down.

Thanks for the comparison with Bootstrap.

[Avi98]

Hi @oliviertassinari , anyone is working on it? Can I take this up?

[oliviertassinari]

@Avi98 Ask @nkrivous. But if you have seen any area of the discussion that isn't clear, let us know! At this point, we were hesitating on a. the aliases direction vs b. not rendering anything if a breakpoint is missing,

So far, we write the CSS of the components mobile-first (up), with rare exceptions for desktop first (down), this happens when it allows simpler CSS.

The issue with b. is that if we don't render anything, the behavior of the components might be unpredictable. This doesn't seem to align with the objective, we still want to the breakpoint but have different names.

If we want to change the breakpoint behavior of the components wouldn't it be better to write an override? I would vote for option a. This option also allows us to implements runtime warnings in case the configuration of the theme is wrong: using a breakpoint name that wasn't defined.

[kmurgic]

This is still an issue in v5. Looks like there has not been any movement on this in a couple years. I implemented custom breakpoints as shown in the example and noticed some peculiar diffs in my snapshot tests for a Tabs component

- @media (max-width:599.95px) {
+ @media (max-width:NaNpx) {

Turned out that tabs was using theme.breakpoints.down('sm') under the hood.

My solution was to add back the breakpoints for xs, sm, md, lg, xl and duplicate the values from other breakpoints, and then hide them from TS. Would be nice if this could be fixed with aliasing. In the meantime, it seems prudent to at least add some sort of note in the documentation.

Perhaps we should start the first suggestion you provided in this comment #21778 (comment). IMO adding an aliases object to the breakpoints (like your second example) feels like a more stable solution, but updating the docs seems good enough.

I'm happy to make a PR to update the docs. I could also work on the second option of adding an aliases property to the breakpoints object, but I might be a little slower getting that PR ready since I don't have a lot of free time right now.