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

CSS Media Queries don't work with Custom breakpoints #21745

Closed
2 tasks done
nkrivous opened this issue Jul 10, 2020 · 24 comments · Fixed by #21759 or #26328
Closed
2 tasks done

CSS Media Queries don't work with Custom breakpoints #21745

nkrivous opened this issue Jul 10, 2020 · 24 comments · Fixed by #21759 or #26328
Labels
bug 🐛 Something doesn't work component: Container The React component good first issue Great for first contributions. Enable to learn the contribution process. priority: important This change can make a difference

Comments

@nkrivous
Copy link
Contributor

nkrivous commented Jul 10, 2020

According to the documentation, we can create custom breakpoints::

const theme = createMuiTheme({
  breakpoints: {
    values: {
      tablet: 640,
      laptop: 1024,
      desktop: 1280,
    },
  },
});

But CSS Media Queries don't understand these new values

theme.breakpoints.up(key)
theme.breakpoints.down(key)
theme.breakpoints.only(key)
theme.breakpoints.between(start, end)
  • 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 😯

The following code

   [theme.breakpoints.down("laptop")]: {
      padding: "10px",
    },

produces the next css

@media (max-width:NaNpx) {
  .makeStyles-root-5 {
    padding: 10px;
  }
}

Context 🔦

I think the reason is in the wrong function createBreakpoints.js because we never update an array of keys.
It should be smth like that

export const predefinedKeys = ['xs', 'sm', 'md', 'lg', 'xl'];

// Keep in mind that @media is inclusive by the CSS specification.
export default function createBreakpoints(breakpoints) {
  const {
    // The breakpoint **start** at this value.
    // For instance with the first breakpoint xs: [xs, sm[.
    values = {
      xs: 0,
      sm: 600,
      md: 960,
      lg: 1280,
      xl: 1920,
    },
    unit = 'px',
    step = 5,
    keys = predefinedKeys,
    ...other
  } = breakpoints;
}
Tech Version
Material-UI v4.11.0
React v16.13.1
@nkrivous nkrivous added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 10, 2020
@oliviertassinari oliviertassinari added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 10, 2020
@oliviertassinari

This comment has been minimized.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed status: waiting for author Issue with insufficient information labels Jul 10, 2020
@oliviertassinari
Copy link
Member

@oliviertassinari
Copy link
Member

@nkrivous What do you think of this fix? Do you want to work on a pull request? :) We would also need to add tests:

diff --git a/packages/material-ui/src/styles/createBreakpoints.js b/packages/material-ui/src/styles/createBreakpoints.js
index b961863c7..e72b304c0 100644
--- a/packages/material-ui/src/styles/createBreakpoints.js
+++ b/packages/material-ui/src/styles/createBreakpoints.js
@@ -1,6 +1,6 @@
 // Sorted ASC by size. That's important.
 // It can't be configured as it's used statically for propTypes.
-export const keys = ['xs', 'sm', 'md', 'lg', 'xl'];
+export const breakpointKeys = ['xs', 'sm', 'md', 'lg', 'xl'];

 // Keep in mind that @media is inclusive by the CSS specification.
 export default function createBreakpoints(breakpoints) {
@@ -19,6 +19,8 @@ export default function createBreakpoints(breakpoints) {
     ...other
   } = breakpoints;

+  const keys = Object.keys(values);
+
   function up(key) {
     const value = typeof values[key] === 'number' ? values[key] : key;
     return `@media (min-width:${value}${unit})`;
@@ -29,8 +31,8 @@ export default function createBreakpoints(breakpoints) {
     const upperbound = values[keys[endIndex]];

     if (endIndex === keys.length) {
-      // xl down applies to all sizes
-      return up('xs');
+      // end index down applies to all sizes
+      return '@media (min-width:0px)';
     }

     const value = typeof upperbound === 'number' && endIndex > 0 ? upperbound : key;
diff --git a/packages/material-ui/src/withWidth/withWidth.js b/packages/material-ui/src/withWidth/withWidth.js
index 542dc37d6..04d081ab7 100644
--- a/packages/material-ui/src/withWidth/withWidth.js
+++ b/packages/material-ui/src/withWidth/withWidth.js
@@ -4,7 +4,7 @@ import { getDisplayName } from '@material-ui/utils';
 import { getThemeProps } from '@material-ui/styles';
 import hoistNonReactStatics from 'hoist-non-react-statics';
 import useTheme from '../styles/useTheme';
-import { keys as breakpointKeys } from '../styles/createBreakpoints';
+import { breakpointKeys } from '../styles/createBreakpoints';
 import useMediaQuery from '../useMediaQuery';

 // By default, returns true if screen width is the same or greater than the given breakpoint.

@oliviertassinari oliviertassinari added priority: important This change can make a difference good first issue Great for first contributions. Enable to learn the contribution process. labels Jul 10, 2020
@nkrivous
Copy link
Contributor Author

@oliviertassinari Thanks for the fast response. I like your fix and I'm going to work on a PR. I looked at the mui code base and noticed that in some components breakpoints are hardcoded.
For example, Tooltip, Toolbar, Tabs, and many others contain the line [theme.breakpoints.up('sm')]: {..}
Even in your fix in the file withWidth.js we refer to predefined breakpoints, not the theme.

Can we solve the problem globally?

  1. Maybe instead of overriding breakpoints, we can use aliases for the first 5, and be able to expand this list? This means that we will have at least 5 breakpoints, and we should not change the existing logic.
alias: {
     tablet: ['xs', 'sm'],
     laptop: ['md', 'lg',],
     desktop: ['xl', 'xxl'],
},
values = {
     ...prdefinedValues,
      xxl: 2560,
},
  1. We can allow the redefinition of breakpoints. But in this case, we should not refer to predefined points in the codebase. We must remove the incompatible propTypes and other hardcoded strings.

We also should remember about components that receive breakpoints in their props: <Grid item xs={12} />

Anyway, I will start with fixing the current bug.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 11, 2020

noticed that in some components breakpoints are hardcoded

Oh right, I have overlooked this aspect. Would it work for you if you still have to include the xs, sm, md, xl, lg values?

Also, shouldn't we warn if a key is provided to the breakpoint methods but not supported in the theme?

We also should remember about components that receive breakpoints in their props:

This is a different problem, I would include it as part of #6140.

@nkrivous
Copy link
Contributor Author

Would it work for you if you still have to include the xs, sm, md, xl, lg values?

As a workaround, this works. But, as a consumer of the API, I don't want to have old values if you give me the opportunity to override something.

Also, shouldn't we warn if a key is provided to the breakpoint methods but not supported in the theme?

Modern frontend developers use TypeScript, which does not allow them to pass not supported values :)

@Guneetgstar
Copy link

The issue is closed but custom break point still doesn't work as described in the docs. Also as @oliviertassinari suggested here to try alpha version to see it working I think this must be reflected in the docs as well.

@HimanshuMamodiya
Copy link

HimanshuMamodiya commented Sep 12, 2020

@oliviertassinari
I am using the same way as mentioned in the docs, with alpha version . still I am not able to access . I have tried by providing keys as well . Any help will be appreciable.

const theme = createMuiTheme({
breakpoints: {
values: {
xs: 0,
tablet: 640,
laptop: 1024,
desktop: 1280,
},
},
});

@Guneetgstar are you able to apply custom break point now . if yes would you please share demo code sandbox link .

@oliviertassinari
Copy link
Member

@HimanshuMamodiya Do you have a reproduction on codesandbox with v5.0.0-alpha.10?

@HimanshuMamodiya
Copy link

@oliviertassinari , actually that break-point is giving the value , but value is not applicable in the div it is still taking the other value.
https://codesandbox.io/s/material-demo-forked-jq49k?file=/demo.js:357-359
const theme = createMuiTheme({
breakpoints: {
values: {
xs: 0,
sm: 600,
md: 700,
ipadl: 900,
lg: 1025
}
}
});

I am not able to post actual code but tried to recreate the scenario. In this my code is still using md breakpoint rather than using ipadl for ipad-landscape mode.

@C5H8NNaO4
Copy link

C5H8NNaO4 commented Sep 30, 2020

This still doesn't seem to be working with grids. The only work-around I found was to use the useWidth hook to augment the xl property of the grid. It would be great if grids would support the custom breakpoints as I'm used to extra-wide monitors.

Edit: The <Hidden> component also doesn't seem to support custom breakpoints.

const width = useWidth();
<Grid item sm={12} lg={6} xl={width=='xxl'?3:6}>
    <Widget symbol={symbol}/>
</Grid>

@oliviertassinari
Copy link
Member

@C5H8NNaO4 I think that #6140 should cover custom breakpoints, see #21589.

@acdoussan
Copy link

acdoussan commented Dec 3, 2020

If anyone stumbles upon this and doesn't want to use alpha / wait for full release, you can get around this by manually passing the breakpoint value. For example, instead of theme.breakpoints.down('tablet') write theme.breakpoints.down(theme.breakpoints.values.tablet)

@mgeduld
Copy link

mgeduld commented Jan 29, 2021

This doesn't work for me: theme.breakpoints.down(theme.breakpoints.values.tablet)

@BetoOrozco95
Copy link

BetoOrozco95 commented Mar 20, 2021

Hello, you need to change the keys too

//Example
const theme = createMuiTheme({

		breakpoints: {
			values: {
				tablet: 640,
				laptop: 1024,
				desktop: 1280
			}
		},
		overrides: {
			MuiCssBaseline: {
				'@global': {
					body: {
						overflow: 'hidden'
					}
				}
			}
		},
		palette: {
			type: 'dark',
			primary: {
				light: '#C2C2C3',
				main: '#323338',
				dark: '#131417'
			},
			secondary: {
				light: '#B8E1D9',
				main: '#129B7F',
				dark: '#056D4F',
				contrastText: '#FFFFFF'
			},
			background: {
				paper: '#262526',
				default: '#1E1D1E'
			},
			error: red
		},
		status: {
			danger: 'orange'
		}
	});

@kongweiying2
Copy link

kongweiying2 commented Apr 29, 2021

Hello, you need to change the keys too […]

Having keys like that doesn't work for me. I'm trying to use the custom key breakpoints on the <Container maxWidth="tablet"/>.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 29, 2021

I'm trying to use the custom key breakpoints on the .

@TheAussieStew Could you open a new issue for this case? We have done our best to handle custom breakpoints, but the API is large, we forgot a few.

It seems that we could make it work with:

diff --git a/packages/material-ui/src/Container/Container.d.ts b/packages/material-ui/src/Container/Container.d.ts
index 2049acda0e..2552905e7c 100644
--- a/packages/material-ui/src/Container/Container.d.ts
+++ b/packages/material-ui/src/Container/Container.d.ts
@@ -1,8 +1,11 @@
 import * as React from 'react';
 import { SxProps } from '@material-ui/system';
 import { Theme } from '../styles';
+import { Breakpoints } from '../styles/createBreakpoints';
 import { OverridableComponent, OverrideProps } from '../OverridableComponent';

+export type BreakpointsKeys = keyof Breakpoints['values'];
+
 export interface ContainerTypeMap<P = {}, D extends React.ElementType = 'div'> {
   props: P & {
     children?: React.ReactNode;
@@ -46,7 +49,7 @@ export interface ContainerTypeMap<P = {}, D extends React.ElementType = 'div'> {
      * Set to `false` to disable `maxWidth`.
      * @default 'lg'
      */
-    maxWidth?: 'xs' | 'sm' | 'md' | 'lg' | 'xl' | false;
+    maxWidth?: BreakpointsKeys | false;
     /**
      * The system prop that allows defining system overrides as well as additional CSS styles.
      */
diff --git a/packages/material-ui/src/Container/Container.js b/packages/material-ui/src/Container/Container.js
index 74dce02f09..aab6ebc70c 100644
--- a/packages/material-ui/src/Container/Container.js
+++ b/packages/material-ui/src/Container/Container.js
@@ -155,7 +155,10 @@ Container.propTypes /* remove-proptypes */ = {
    * Set to `false` to disable `maxWidth`.
    * @default 'lg'
    */
-  maxWidth: PropTypes.oneOf(['lg', 'md', 'sm', 'xl', 'xs', false]),
+  maxWidth: PropTypes /* @typescript-to-proptypes-ignore */.oneOfType([
+    PropTypes.oneOf(['lg', 'md', 'sm', 'xl', 'xs']),
+    PropTypes.string,
+  ]),
   /**
    * The system prop that allows defining system overrides as well as additional CSS styles.
    */
import * as React from 'react';
import { Container, ThemeProvider, createTheme } from '@material-ui/core';

declare module '@material-ui/core/styles' {
  interface BreakpointOverrides {
    xs: false; // removes the `xs` breakpoint
    sm: false;
    md: false;
    lg: false;
    xl: false;
    tablet: true; // adds the `tablet` breakpoint
    laptop: true;
    desktop: true;
  }
}

const theme = createTheme({
  breakpoints: {
    values: {
      tablet: 640,
      laptop: 1024,
      desktop: 1280,
    },
  },
  components: {
    MuiContainer:{
      defaultProps: {
        maxWidth: 'laptop',
      },
    },
  },
});

export default () => (
  <ThemeProvider theme={theme}>
    hello
    <Container maxWidth="tablet">
      yooo
    </Container>
  </ThemeProvider>
);

@alanszp
Copy link
Contributor

alanszp commented May 16, 2021

@oliviertassinari would like to see this in the next alpha release. Would you like me to make a PR?

@oliviertassinari
Copy link
Member

@alanszp Yes, a pull request would be great.

@alanszp
Copy link
Contributor

alanszp commented May 16, 2021

@oliviertassinari done! I spotted another component with the same issue: Dialog. Wil make another PR

@alanszp
Copy link
Contributor

alanszp commented May 16, 2021

@oliviertassinari just created another issue for the Dialog. Wolud like to know what to do with the autogenerated propTypes, and which would be the best way to change them as you did on the Container one.

#26330

@azz0r
Copy link

azz0r commented Mar 2, 2022

Hello, am I right in saying this isn't fixed still in v4? The comments on the last closure indicate you are just supporting it for 5?

@eoinpayne
Copy link

This is still not working for me on v5

@tenshouuu
Copy link

tenshouuu commented Jun 9, 2023

guys, you can use createBreakpoints function from "@mui/system" package

const theme = createTheme({ breakpoints: createBreakpoints({ values: CUSTOM_BREAKPOINTS, }), }),

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: Container The React component good first issue Great for first contributions. Enable to learn the contribution process. priority: important This change can make a difference
Projects
None yet