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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[system] styled should accept sx syntax #28678

Closed
1 task done
eric-burel opened this issue Sep 29, 2021 · 10 comments 路 Fixed by #29833
Closed
1 task done

[system] styled should accept sx syntax #28678

eric-burel opened this issue Sep 29, 2021 · 10 comments 路 Fixed by #29833
Labels
discussion new feature New feature or request package: system Specific to @mui/system waiting for 馃憤 Waiting for upvotes

Comments

@eric-burel
Copy link
Contributor

eric-burel commented Sep 29, 2021

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 馃挕

styled and sx should accept the same set of props.

Examples 馃寛

const WrapperPaper = styled(Paper)({
  my: 2,
  mx: "auto",
  padding: 2,
  minWidth: "600px",
});
// is different from
<Paper sx={{ my: 2, mx: "auto", padding: 2, minWidth: "600px" }}>

The WrapperPaper component doesn't seem to apply the shortcuts
image

TypeScript accepts those mx and my props though in the styled call, they are simply not translated afterward. The sx version works fine, but leads to code duplication in my use case.

Motivation 馃敠

I'd expect both to have the same API.

@eric-burel eric-burel added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 29, 2021
@siriwatknp siriwatknp added package: system Specific to @mui/system new feature New feature or request and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 29, 2021
@siriwatknp
Copy link
Member

@mnajdova I also have the same feeling here (after using a lot of styled and sx). Sometimes, I mistakenly write sx syntax inside styled and figure out later that it does not work 馃槀.

I think it seems possible to accept sx directly in styled but the only concern is about performance gain.

@siriwatknp siriwatknp changed the title sx and styled have different APIs / behaviour regarding shortcuts [system] styled should accept sx syntax Sep 29, 2021
@mnajdova
Copy link
Member

Thanks for the feedback.

The downside I see with supporting this API, is that we are no longer compatible with other frameworks' styled() API. For example, adding margin: 1 would no longer add margin: "1px". This is the main reason why initially we didn't change the API. People are mostly already aware of how to use the styled() API, so it would be surprising if it didn't work the same way. More over, if they are moving components already created with a styled() API, the styles would break.

On the other hand, the sx prop is a new/additional API, which main and sole purpose is to allow a fast/inline way for mapping theme values to styles. Having this said, the above code makes sense.

I would propose we wait and see how this resonates with the community. I would be happy to change how styled() work if this proves to be a better alternative.

@eric-burel
Copy link
Contributor Author

eric-burel commented Sep 29, 2021

Yeah I understand, I was indeed wondering whether "margin: 1" would add 1px, or spacing(1). It could indeed require a new API, for instance, exporting an sx function:

// sx would be a thin wrapper converting the input and calling `styled` under the hood
const WrapperPaper = sx(Paper)({
    margin: 1; // behaves as sx prop, so 1 => theme.spacing(1) (and not 1px)
})

Edit: also there might be smth wrong in the typings, because styled will accept non-existing attributes, like mx and my, as far as I understand it shouldn't.

I may try to send a PR to the documentation. Another slightly related question: it's not documented how you pass props to styled. Emotion uses this pattern:

styled("div")({
   backgroundColor: props => props.myColorProps
})

While tss-react makeStyles is using:

makeStyles((theme, props) => ({
     backgroundColor: props.myColorProps
})

and Material UI makeStyles is using:

makeStyles({
    backgroundColor: props => props.myColorProps
})

and finally, sx being a prop, will use:

sx={{
    backgroundColor: props.myColorProps
}}

That's a source of confusion when migrating :) we broke one of our component due to this. It would be worth documenting as well imo, I could take the occasion to PR that as well.

@mnajdova
Copy link
Member

@eric-burel thanks for the feedback. Would be great if you can propose a PR with the pain points you had, it would help a lot 馃檹 We can discuss the details on the PR.

@eric-burel
Copy link
Contributor Author

eric-burel commented Sep 30, 2021

An sx function that behaves like a mix between styled and sx prop could be defined like this:

// hoc with syntax similar to styled but accepting sx styles
const sx = (C) => (sxStyle) => (props) => {
   return <C sx={typeof sxStyle === "function" ? sxStyle(props) : sxStyle} {...props} />
}
// usage
const StyledFooBasic= sx(Foo)({ mx: 1, backgroundColor: "red", padding: theme => theme.spacing(1) })
) 
const StyledFooWithProps = sx(Foo)(
  props => ({ mx: 1, backgroundColor: props.myCustomColor, padding: theme => theme.spacing(1) })
) 

That's very draftish and lacks typings, but I think this could be done already in user-land. I haven't yet switched my main project to mui v5 but I might give this a shot later on.

@mnajdova
Copy link
Member

Note also that the @mui/system exports a styleFunctionSx utility that accepts props (together with theme) for adding the behavior of the sx prop. It can be used as well in user land.

@mnajdova
Copy link
Member

mnajdova commented Nov 17, 2021

I came back to this again after reading a post yesterday about how developers really like the sx syntax and want to use it everywhere. Here is a very simple POC that can be added in a user-land that would allow this - https://codesandbox.io/s/crazy-marco-m9372?file=/src/App.tsx

import * as React from "react";
import {
  unstable_styleFunctionSx as styleFunctionSx,
  SxProps
} from "@mui/system";
import { styled, Theme, CSSObject } from "@mui/material/styles";

const sx = (styles: SxProps<Theme>, theme: Theme) => {
  return styleFunctionSx({ sx: styles, theme }) as CSSObject;
};

const Button = styled("button")(({ theme }) =>
  sx(
    {
      bgcolor: "primary.dark",
      color: "common.white",
      borderRadius: 1,
      border: "none",
      p: 1
    },
    theme
  )
);

export default function App() {
  return <Button>Isn't this awesome?</Button>;
}

Another option would be to export a util styledx() that would accept this syntax.

Note: This works only with CSS object syntax.

cc @siriwatknp @oliviertassinari what are your thoughts?

@siriwatknp
Copy link
Member

siriwatknp commented Nov 19, 2021

@mnajdova Regarding the API, I can think of 2 options:

  1. expose a global sx function (if I understand @mnajdova correctly, the difference is sx(styles) return function instead of object)
// I believe that this is the function we export, right?
const sx = (styles: SxProps<Theme>) => ({ theme: Theme }) => {
  return styleFunctionSx({ sx: styles, theme }) as CSSObject;
};

// This is how developer use the function.
const Button = styled("button")(
  sx({
    bgcolor: "primary.dark",
    color: "common.white",
    borderRadius: 1,
    border: "none",
    p: 1
  })
);

I don't have to get the theme and pass it to the sx function again. I expect that this should work.

Also, what I suggest should work with all of these scenarios:

const Button = styled("button")(
  sx({
    bgcolor: "primary.dark",
    color: "common.white",
    borderRadius: 1,
    border: "none",
    p: 1
  })
);

const Button = styled("button")(
  sx(({ theme }) => ({
    bgcolor: "primary.dark",
    color: "common.white",
    borderRadius: 1,
    border: "none",
    p: 1
  }))
);

const Button = styled("button")(
  ({ theme }) => sx({
    bgcolor: "primary.dark",
    color: "common.white",
    borderRadius: 1,
    border: "none",
    p: 1
  }) // emotion & styled-components support function inside a function, so this is safe.
);
  1. sx is ready to use from the callback. what I like about this is that I don't need to always import the sx. I can just do:
import { styled } from '@mui/material/styles';

const Button = styled('button')(({ sx }) => (
  sx({
    bgcolor: "primary.dark",
    color: "common.white",
    borderRadius: 1,
    border: "none",
    p: 1
  })
))

Is the second option possible?

@mnajdova
Copy link
Member

I don't have to get the theme and pass it to the sx function again. I expect that this should work.

Will need to check to see how this could work.

Also, what I suggest should work with all of these scenarios:

I would try to keep the API to minimum, I would say the first and last APIs are enough.

Is the second option possible?

Yes, but there would be the conflict between the sx prop and the sx function, we would need to override it which can be surprising. What's wrong with importing a module? Otherwise we need to again invent some new API. With the import is clear what is expected - I am preprocessing the input of the styled() utility.

@mnajdova
Copy link
Member

mnajdova commented Nov 22, 2021

One other advantage of exporting the sx utility is that you can use it even outside of the styled() utility, for example in the theme's variants, mixins or some custom styling setup that developers may have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion new feature New feature or request package: system Specific to @mui/system waiting for 馃憤 Waiting for upvotes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants