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

[Box] doesn't apply maxWidth prop #25771

Closed
PetrShchukin opened this issue Apr 14, 2021 · 10 comments · Fixed by #26984
Closed

[Box] doesn't apply maxWidth prop #25771

PetrShchukin opened this issue Apr 14, 2021 · 10 comments · Fixed by #26984
Assignees
Labels
component: Box The React component. good first issue Great for first contributions. Enable to learn the contribution process. package: system Specific to @mui/system

Comments

@PetrShchukin
Copy link

Box doesn't apply maxWidth prop

    <Box maxWidth="xs">
      Hello World
    </Box>

CodeSandBox:
https://codesandbox.io/s/hardcore-platform-tyfvw?file=/index.js:131-183

Mui version: v4.9.8 ✓

@PetrShchukin PetrShchukin added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 14, 2021
@oliviertassinari
Copy link
Member

@PetrShchukin What lead you to believe this API is supported?

@oliviertassinari oliviertassinari added component: Box The React component. 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 Apr 14, 2021
@PetrShchukin
Copy link
Author

PetrShchukin commented Apr 15, 2021

In this page https://material-ui.com/components/box/#box, the latest sentence is "Any other properties supplied will be used by the style functions or spread to the root element." I'd expect maxWidth to be support as width is supported.

@oliviertassinari oliviertassinari self-assigned this Apr 15, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 15, 2021

@PetrShchukin Ok thanks. I can't think of any way we could have solved the root cause, so I'm simply going to answer the direct question.

xs is a breakpoint value (and is 0px), it's not accepted as a final value. This would work

import React from "react";
import ReactDOM from "react-dom";
import Box from "@material-ui/core/Box";

function App() {
  return (
    <Box maxWidth={100} bgcolor="red">
      Hello World
    </Box>
  );
}

ReactDOM.render(<App />, document.querySelector("#app"));

https://codesandbox.io/s/quirky-moon-ss53u

In v5, you can also do:

import React from "react";
import ReactDOM from "react-dom";
import Box from "@material-ui/core/Box";

function App() {
  return (
    <Box maxWidth={(theme) => theme.breakpoints.values.sm} bgcolor="red">
      Hello World
    </Box>
  );
}

ReactDOM.render(<App />, document.querySelector("#app"));

https://codesandbox.io/s/agitated-agnesi-f5rfs

@oliviertassinari oliviertassinari added support: question Community support but can be turned into an improvement and removed status: waiting for author Issue with insufficient information labels Apr 15, 2021
@oliviertassinari oliviertassinari changed the title Box doesn't apply maxWidth prop [Box] doesn't apply maxWidth prop Apr 15, 2021
@matheusmb
Copy link

matheusmb commented May 20, 2021

@oliviertassinari how to use your v5 example with Typescript? I tried here https://codesandbox.io/s/brave-pine-mhmf2?file=/index.tsx but I'm receiving the following TS Error:

  Overload 1 of 2, '(props: { component: ElementType<any>; } & { borderTop?: ResponsiveStyleValue<string | number | (string & {}) | MaxWidth<Key>[]>; borderRight?: ResponsiveStyleValue<...>; ... 62 more ...; textAlign?: ResponsiveStyleValue<...>; } & ... 4 more ... & Pick<...>): Element', gave the following error.
    Type '(theme: Theme) => number' is not assignable to type 'ResponsiveStyleValue<string | number | (string & {}) | MaxWidth<Key>[]>'.
      Type '(theme: Theme) => number' is not assignable to type '{ [key: string]: string | number | (string & {}) | MaxWidth<Key>[]; }'.
        Index signature is missing in type '(theme: Theme) => number'.
  Overload 2 of 2, '(props: DefaultComponentProps<BoxTypeMap<{}, "div">>): Element', gave the following error.
    Type '(theme: Theme) => number' is not assignable to type 'ResponsiveStyleValue<string | number | (string & {}) | MaxWidth<Key>[]>'.
      Type '(theme: Theme) => number' is not assignable to type '{ [key: string]: string | number | (string & {}) | MaxWidth<Key>[]; }'.ts(2769)
index.tsx(8, 19): Did you mean to call this expression?

Is the type definition correct?

It would be nice use the breakpoints tokens as strings, just as Chakra-UI has implemented, e.g.:

import React from "react";
import ReactDOM from "react-dom";
import Box from "@material-ui/core/Box";

function App() {
  return (
    <Box maxWidth="sm" bgcolor="red">
      Hello World
    </Box>
  );
}

ReactDOM.render(<App />, document.querySelector("#app"));

@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer package: system Specific to @mui/system and removed support: question Community support but can be turned into an improvement design This is about UI or UX design, please involve a designer labels May 22, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented May 22, 2021

@matheusmb Thanks for raising the TypeScript issue. I can reproduce it too. @mnajdova should we fix it?

diff --git a/packages/material-ui-system/src/index.d.ts b/packages/material-ui-system/src/index.d.ts
index 6473f986fc..6d37237c87 100644
--- a/packages/material-ui-system/src/index.d.ts
+++ b/packages/material-ui-system/src/index.d.ts
@@ -4,7 +4,7 @@ import { CSSProperties } from './CSSProperties';
 import {
   OverwriteCSSProperties,
   AliasesCSSProperties,
-  StandardCSSProperties,
+  AllSystemCSSProperties,
 } from './styleFunctionSx';
 // disable automatic export
 export {};
@@ -291,36 +291,34 @@ export interface CSSOthersObjectForCSSObject {
   [propertiesName: string]: CSSInterpolation;
 }

-export type CustomSystemProps = OverwriteCSSProperties & AliasesCSSProperties;
+export interface CustomSystemProps extends AliasesCSSProperties, OverwriteCSSProperties {}

-export type SimpleSystemKeys = keyof Omit<
-  PropsFor<
-    ComposedStyleFunction<
-      [
-        typeof borders,
-        typeof display,
-        typeof flexbox,
-        typeof grid,
-        typeof palette,
-        typeof positions,
-        typeof shadows,
-        typeof sizing,
-        typeof spacing,
-        typeof typography,
-      ]
-    >
-  >,
-  keyof CustomSystemProps
+export type SimpleSystemKeys = keyof PropsFor<
+  ComposedStyleFunction<
+    [
+      typeof borders,
+      typeof display,
+      typeof flexbox,
+      typeof grid,
+      typeof palette,
+      typeof positions,
+      typeof shadows,
+      typeof sizing,
+      typeof spacing,
+      typeof typography,
+    ]
+  >
 >;

-// The SimpleSystemKeys are subset of the StandardCSSProperties, so this should be ok
-// This is needed as these are used as keys inside StandardCSSProperties
-type StandardSystemKeys = Extract<SimpleSystemKeys, keyof StandardCSSProperties>;
+// The SimpleSystemKeys are subset of the AllSystemCSSProperties, so this should be ok
+// This is needed as these are used as keys inside AllSystemCSSProperties
+type StandardSystemKeys = Extract<SimpleSystemKeys, keyof AllSystemCSSProperties>;

-export type SystemProps = {
-  [K in StandardSystemKeys]?: ResponsiveStyleValue<StandardCSSProperties[K]>;
-} &
-  CustomSystemProps;
+export type SystemProps<Theme extends object = {}> = {
+  [K in StandardSystemKeys]?:
+    | ResponsiveStyleValue<AllSystemCSSProperties[K]>
+    | ((theme: Theme) => ResponsiveStyleValue<AllSystemCSSProperties[K]>);
+};

 export {
   default as unstable_styleFunctionSx,
diff --git a/packages/material-ui-system/src/styleFunctionSx/styleFunctionSx.d.ts b/packages/material-ui-system/src/styleFunctionSx/styleFunctionSx.
d.ts
index 2a30a79bcc..5800fbedc3 100644
--- a/packages/material-ui-system/src/styleFunctionSx/styleFunctionSx.d.ts
+++ b/packages/material-ui-system/src/styleFunctionSx/styleFunctionSx.d.ts
@@ -26,13 +26,13 @@ export interface CSSSelectorObject<Theme extends object = {}> {

 /**
  * Map of all available CSS properties (including aliases) and their raw value.
- * Only used internally to map CCS properties to input types (responsive value,
+ * Only used internally to map CSS properties to input types (responsive value,
  * theme function or nested) in `SystemCssProperties`.
  */
 export interface AllSystemCSSProperties
   extends Omit<StandardCSSProperties, keyof OverwriteCSSProperties>,
-    AliasesCSSProperties,
-    OverwriteCSSProperties {}
+    OverwriteCSSProperties,
+    AliasesCSSProperties {}

 export type SystemCssProperties<Theme extends object = {}> = {
   [K in keyof AllSystemCSSProperties]:
diff --git a/packages/material-ui/src/Box/Box.d.ts b/packages/material-ui/src/Box/Box.d.ts
index d9c41651a2..f4a5104ede 100644
--- a/packages/material-ui/src/Box/Box.d.ts
+++ b/packages/material-ui/src/Box/Box.d.ts
@@ -5,7 +5,7 @@ import { Theme } from '../styles/createTheme';

 export interface BoxTypeMap<P = {}, D extends React.ElementType = 'div'> {
   props: P &
-    SystemProps & {
+    SystemProps<Theme> & {
       children?: React.ReactNode;
       component?: React.ElementType;
       ref?: React.Ref<unknown>;
diff --git a/packages/material-ui/src/Grid/Grid.d.ts b/packages/material-ui/src/Grid/Grid.d.ts
index f7e35fbc2f..4a5b602225 100644
--- a/packages/material-ui/src/Grid/Grid.d.ts
+++ b/packages/material-ui/src/Grid/Grid.d.ts
@@ -14,7 +14,7 @@ export type GridSize = 'auto' | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12

 export interface GridTypeMap<P = {}, D extends React.ElementType = 'div'> {
   props: P &
-    SystemProps & {
+    SystemProps<Theme> & {
       /**
        * The content of the component.
        */
diff --git a/packages/material-ui/src/Stack/Stack.d.ts b/packages/material-ui/src/Stack/Stack.d.ts
index 1703f2a4b5..e65d46be71 100644
--- a/packages/material-ui/src/Stack/Stack.d.ts
+++ b/packages/material-ui/src/Stack/Stack.d.ts
@@ -5,7 +5,7 @@ import { Theme } from '../styles/createTheme';

 export interface StackTypeMap<P = {}, D extends React.ElementType = 'div'> {
   props: P &
-    SystemProps & {
+    SystemProps<Theme> & {
       ref?: React.Ref<unknown>;
       /**
        * The content of the component.
diff --git a/packages/material-ui/src/Typography/Typography.d.ts b/packages/material-ui/src/Typography/Typography.d.ts
index eecbc311fb..721e634f5b 100644
--- a/packages/material-ui/src/Typography/Typography.d.ts
+++ b/packages/material-ui/src/Typography/Typography.d.ts
@@ -10,7 +10,7 @@ export interface TypographyPropsVariantOverrides {}

 export interface TypographyTypeMap<P = {}, D extends React.ElementType = 'span'> {
   props: P &
-    SystemProps & {
+    SystemProps<Theme> & {
       /**
        * Set the text-align on the component.
        * @default 'inherit'

Regarding native support of the breakpoint values. It could make sense. We have these props on the Dialog and Container components. We would need to do:

diff --git a/packages/material-ui-system/src/sizing.js b/packages/material-ui-system/src/sizing.js
index f957b440be..f9227c74fe 100644
--- a/packages/material-ui-system/src/sizing.js
+++ b/packages/material-ui-system/src/sizing.js
@@ -1,5 +1,6 @@
 import style from './style';
 import compose from './compose';
+import { handleBreakpoints } from './breakpoints';

 function transform(value) {
   return value <= 1 ? `${value * 100}%` : value;
@@ -10,10 +11,20 @@ export const width = style({
   transform,
 });

-export const maxWidth = style({
-  prop: 'maxWidth',
-  transform,
-});
+export const maxWidth = (props) => {
+  if (props.maxWidth) {
+    const styleFromPropValue = (propValue) => {
+      const breakpoint = props.theme.breakpoints.values[propValue];
+      return {
+        maxWidth: breakpoint || transform(propValue),
+      };
+    }
+    return handleBreakpoints(props, props.maxWidth, styleFromPropValue);
+  }
+  return null;
+};
+
+maxWidth.filterProps = ['maxWidth'];

 export const minWidth = style({
   prop: 'minWidth',

to have

import React from 'react';
import Box from '@material-ui/core/Box';

export default function App() {
  return (
    <Box maxWidth="sm" bgcolor="red" m={[1, 2]}>
      Hello World
    </Box>
  );
}

it's for instance supported in Tailwind: https://tailwindcss.com/docs/max-width.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label May 26, 2021
@mnajdova
Copy link
Member

I like this idea. Using the breakpoints as values makes sense 👍

@ansh-saini
Copy link
Contributor

@oliviertassinari Can I work on this?

@ansh-saini
Copy link
Contributor

Hey @oliviertassinari I opened a PR which only fixes the TypeScript issue.
I have not implemented the tailwind kind of functionality on the maxWidth prop. Should I implement it in a different PR?

@oliviertassinari
Copy link
Member

Should I implement it in a different PR?

@ansh-saini Doing it in a second PR sounds great.

@ansh-saini
Copy link
Contributor

Alright, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Box The React component. good first issue Great for first contributions. Enable to learn the contribution process. package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants