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

Unable to customise theme breakpoints #26648

Closed
2 tasks done
robphoenix opened this issue Jun 8, 2021 · 6 comments · Fixed by #26684
Closed
2 tasks done

Unable to customise theme breakpoints #26648

robphoenix opened this issue Jun 8, 2021 · 6 comments · Fixed by #26684
Assignees
Labels
good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@robphoenix
Copy link
Contributor

robphoenix commented Jun 8, 2021

  • 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 😯

No longer able to customise theme breakpoints.

Expected Behavior 🤔

Be able to import the BreakpointOptions type from core and use it to pass in custom breakpoints to theme.

Steps to Reproduce 🕹

https://codesandbox.io/s/basicalerts-material-demo-forked-7qnte?file=/demo.tsx:9-27

import { BreakpointsOptions } from "@material-ui/core/styles/createBreakpoints";

const getBreakpoints = (theme: CustomerUITheme): BreakpointsOptions => ({
    values: theme.breakpoints,
});

shows error Cannot find module '@material-ui/core/styles/createBreakpoints

there doesn't appear to be any error passing breakpoints into the createTheme function, and i believe createBreakpoints has moved to the system package.
Do we need to install @material-ui/system to be able to use the BreakpointsOptions type?

@robphoenix robphoenix added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 8, 2021
@mnajdova
Copy link
Member

mnajdova commented Jun 8, 2021

@robphoenix thanks for the report. Looks like I re-exported only the typings which were previously exported from @material-ui/core/styles. I would suggest we re-export these typings directly from here:

index 67019cb6e5..64c2eaf38d 100644
--- a/packages/material-ui/src/styles/index.d.ts
+++ b/packages/material-ui/src/styles/index.d.ts
@@ -35,6 +35,9 @@ export {
   Direction,
   Breakpoint,
   BreakpointOverrides,
+  BreakpointValues,
+  Breakpoints,
+  BreakpointsOptions,
   CreateMUIStyled,
   CSSObject,
 } from '@material-ui/system';

In this case you would be able to do:

-import { BreakpointsOptions } from "@material-ui/core/styles/createBreakpoints";
+import { BreakpointsOptions } from "@material-ui/core/styles";

 const getBreakpoints = (theme: CustomerUITheme): BreakpointsOptions => ({
     values: theme.breakpoints,
 });

@eps1lon @oliviertassinari should we still support exports from modules like: @material-ui/core/styles/createBreakpoints? I thought the longest path we support is three, for example till @material-ui/core/styles. Should I list all breaking changes related to these moved files?

@eps1lon
Copy link
Member

eps1lon commented Jun 9, 2021

should we still support exports from modules like: @material-ui/core/styles/createBreakpoints?

We can for internal usage. They just aren't useable for the public but we probably should make that happen as well.

@mnajdova
Copy link
Member

mnajdova commented Jun 9, 2021

We can for internal usage. They just aren't useable for the public but we probably should make that happen as well.

I thought that we should support up to level 3, otherwise we are making public the folder structure that we have internally, which with changes like this will break 😕 Also not supporting levels bigger than 3 will allow us to have private types as well.

@oliviertassinari oliviertassinari removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 9, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 9, 2021

The proposed patch looks great. I would even not make BreakpointValues public but and inline it for consistency with the other keys:

diff --git a/packages/material-ui-system/src/createTheme/createBreakpoints.d.ts b/packages/material-ui-system/src/createTheme/createBreakpoints.d.ts
index 6389985a74..3a9544acb7 100644
--- a/packages/material-ui-system/src/createTheme/createBreakpoints.d.ts
+++ b/packages/material-ui-system/src/createTheme/createBreakpoints.d.ts
@@ -6,12 +6,11 @@ export type Breakpoint = OverridableStringUnion<
   'xs' | 'sm' | 'md' | 'lg' | 'xl',
   BreakpointOverrides
 >;
-export type BreakpointValues = { [key in Breakpoint]: number };
 export const keys: Breakpoint[];

 export interface Breakpoints {
   keys: Breakpoint[];
-  values: BreakpointValues;
+  values: { [key in Breakpoint]: number };
   up: (key: Breakpoint | number) => string;
   down: (key: Breakpoint | number) => string;
   between: (start: Breakpoint | number, end: Breakpoint | number) => string;

Regarding the side questions. @material-ui/core/styles/createBreakpoints was never allowed as an import path (AFAIK). It's private. With #26254 we can enforce to make them unreachable.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Jun 9, 2021
@eps1lon
Copy link
Member

eps1lon commented Jun 10, 2021

I thought that we should support up to level 3, otherwise we are making public the folder structure that we have internally, which with changes like this will break confused

I wanted to say that we should make it public via public path. So re-export it from something at level 2 or 3. Not tell people to export from level >=4

@mnajdova
Copy link
Member

I wanted to say that we should make it public via public path. So re-export it from something at level 2 or 3. Not tell people to export from level >=4

Got it, I misunderstood your comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants