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] Add sx prop #23053

Merged
merged 74 commits into from
Oct 22, 2020
Merged

[Box] Add sx prop #23053

merged 74 commits into from
Oct 22, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Oct 14, 2020

Description

This PR adds the support for sx prop on the Box component. It is inspired by Theme UI's sx prop.

Motivation

The Box component currently supports a restricted set of CSS properties on its props API. Most of these properties, which are part of the design system are mapped to specific keys inside the theme. While these properties are useful and allow clean API for the most common scenarios, we don't have support for all CSS props, which makes developers inconsistently use some of them as props, and some of them via external classes defined differently.

In order to solve this problem, we are adding additional property sx, which supports inside all system props that are available as props, but also all other CSS properties. In addition to this, the breakpoints API, which is available as object or array:

{
  border: [1, 2, 3]
}
// or
{
  border: {
    xs: 1,
    sm: 2,
    md: 3
  },
}

// resolves to
{
  '@media (min-width:0px)': { border: '1px solid' },
  '@media (min-width:600px)': { border: '2px solid' },
  '@media (min-width:960px)': { border: '3px solid' },
}

is available on all properties, system and regular CSS.

Another problem that we are solving with this is, allowing developers to define system props on pseudo-elements & selectors.

We expect with this change, that developers will move to the sx prop for defining all styles for the Box as one recommended way to be used. In a follow up PR we will deprecate the existing Box system properties.

Here is an example:

    <Box
      sx={{
        width: 300, // system value
        height: 300, // system value
        bgcolor: 'primary.dark', // system value
        ':hover': { // css pseudo selector
          backgroundColor: 'primary.main', // system value
          opacity: [0.9, 0.8, 0.7], // css value with support of breakpoints API
        },
      }}
    />

TODO:

  • Add tests
  • Add benchmark for the prop

Closes #17297

@mnajdova
Copy link
Member Author

So why do we need individual props? We're bloating the API so much which adds so much choice that any development speed you think you add is offset by the amount of choices and docs one has to read.

Why do we *need border if we already have sx?

I'd really suggest you take a look at the full styling API we're currently supporting. There's no comprehensive documentation for how to style our components. It's all over the place with almost a dozen of APIs that are at this point impossible to teach or learn.

@eps1lon agree I will create a follow up PR deprecate the existing Box props in favor of sx. I agree that it's confusing why we have more than one API for the same thing, plus once we bring the sx prop in all core components, it would be great that we will need to have documentation only for this prop.

The next related PRs will be for deprecating the props and improving the system documentation

You can find the test cases for the above feedback in https://github.com/oliviertassinari/material-ui/commits/feedback-system. It's not directly related to the introduction of the sx but I figure that this PR is progressively being taken down into smaller bits, used as a hub for prototyping so, hopefully, we can turn some of the above feedback into new issues (and look at them in the future).

Note that all/most of these pain points are a legacy from my initial work on the system. Great work on the pull request!

@oliviertassinari let me create issues for the problems you've found so that we can tackle them one by one. I fixed the number 3. but there are still related issues with the breakpoints - #16528 that will be solved separately. I want to keep this PR solely for adding support for the sx prop, otherwise it will grow too big too fast :)

@mnajdova
Copy link
Member Author

@oliviertassinari here is a summary for the issues:

  1. There are cases where you want to get a fraction of the spacing value (e.g; p: 0.5), like for applying it on the left and right sides. This is supported when the spacing is a function or a number but not when an array. Should we support it? Mind that Tailwind's default spacing value is 4px, which is also the smallest unit Material Design grid unit, our default spacing value is 8px. Up until 24px, I think that it makes sense to support increments of 4 pixels, after this threshold, the difference is barely perceptible. If we don't want to support it, I think that we should add a test case. I would be leaning toward not supporting it and raising about it.

Reported #23187


  1. When using the responsive object API, I have made a mistake and got the {} nesting wrong. I ended up with:
<Box
  sx={{
    width: {
      xs: 100,
      color: 'red',
    }
  }}
/>

I was surprised to see the color red style applied.

Not sure what we expect to happen here, but let me open an issue on it after we have the sx prop as an API.


  1. I have tried to combine responsive values for mx and mb at the same time, it failed, only the last key is taken into account
      mx: { xs: "auto", sm: 0 },
      mb: { xs: 1, sm: 0 },

Related to #16528. This bug prevented me to finish the implementation of the design.

Partially fixed, #16528 is relevant for the rest of the issue


  1. I have tried to understand what would each prop of the system do with IntelliSense, unfortunately, all I go was any. I was expecting something similar to:
Capture d’écran 2020-10-20 à 20 52 56

Related to #15451. @eps1lon I think that we will need your help on this one once the API stabilizes :)

Let me open an issue on this after we have the support for the sx prop and deprecation for the other Box props.


  1. I'm not a big fan of having Box everywhere in the template. While moving the support of the prop to the core components will help with the semantic, I think that instead of
        <Box
          component="img"
          sx={{
            display: "block",
            mr: { sm: 3 },
            mx: { xs: "auto", sm: 0 },
            mb: { xs: 1, sm: 0 },
            width: { xs: "4rem", sm: "6rem" },
            height: { xs: "4rem", sm: "6rem" },
            borderRadius: "50%"
          }}
          src="https://tailwindcss.com/_next/static/media/erin-lindford.4804b52007ca82ebe9999d19c717b44d.jpg"
        />

it would be better to have:

        <img
          sx={{
            display: "block",
            mr: { sm: 3 },
            mx: { xs: "auto", sm: 0 },
            mb: { xs: 1, sm: 0 },
            width: { xs: "4rem", sm: "6rem" },
            height: { xs: "4rem", sm: "6rem" },
            borderRadius: "50%"
          }}
          src="https://tailwindcss.com/_next/static/media/erin-lindford.4804b52007ca82ebe9999d19c717b44d.jpg"
        />

For instance https://xstyled.dev/docs/emotion/#use-css-prop or https://theme-ui.com/sx-prop. It seems that we could only support it with emotion, which is OK-ish as it's the default engine 🙃.

Agree, let me open an issue on this after the have the sx prop merged.


  1. Something is off with the typography. I had to repeat the font family, font size, line-height, etc. I would suggest we introduce a font prop to pull from theme.typography.x. This would also solve [system] Using responsiveFontSizes results in inconsistency between <Typography> and <Box> Component #17504.
<Box sx={{ font: "body1" }} />

Reported #23190


  1. I wonder if we shouldn't change the border radius extrapolation from the theme, right now, we need to do:
<Box sx={{ borderRadius: 'borderRadius' }} />

what if we where using theme.shape.borderRadius as we are using theme.spacing, as a multiplication factor?

Reported #23188

  1. I'm not too sure about this one. It's quite frequent to find width: theme.spacing(x) using the codebase, right now, the width's value is directly converted to pixels, should it be transformed by the spacing unit first? Maybe a bad idea.

I wouldn't change anything regarding it at this moment, but we could create an RFC to ask what people think. I think the spacing is usually a small value instended to be used for paddings/margins, not sure how much it would make sense with something like width/height. But maybe we can have some theme.scale value that can be used everywhere, maybe with the default value of 4px.. I am not completely sure on this, but let's create an RFC and ask what developers think.


  1. I'm not very keen on seeing:
          <Box
            sx={{
              fontWeight: "fontWeightRegular",
            }}
          >

I wonder if we would change it to

          <Box
            sx={{
              fontWeight: "regular",
            }}
          >

while still pulling from theme.typography.x (so we can pull from nested variants) or theme.typography.fontWeightX. Related to #17615.

Reported #23192


  1. TypeScript complains about the src prop in:
        <Box
          component="img"
          src="https://tailwindcss.com/_next/static/media/erin-lindford.4804b52007ca82ebe9999d19c717b44d.jpg"
        />

Related to #16843.

Left comment with this use case on the issue as well.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I think that we are going in the right direction with this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Box The React component. package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[system] Settings both X and Y for overflow on Box does not work on Safari
5 participants