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

[system] Support sx on all primitive elements #23220

Open
mnajdova opened this issue Oct 23, 2020 · 12 comments
Open

[system] Support sx on all primitive elements #23220

mnajdova opened this issue Oct 23, 2020 · 12 comments
Labels
new feature New feature or request package: pigment-css Specific to @pigment-css/* package: system Specific to @mui/system

Comments

@mnajdova
Copy link
Member

mnajdova commented Oct 23, 2020

Summary 💡

It would be great if we can support the sx props on the primitives with jsx pragma. That would basically help us to replace

<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://material-ui.com/static/logo_raw.svg"
/>

with

<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://material-ui.com/static/logo_raw.svg"
/>

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 🙃.

Motivation 🔦

This would help us to move away from templates that would use the Box component everywhere. On the other hand, we plan to support the sx prop on all core components, so it may not be that bad, but still, I think this is something that will bring us in the right direction.

Raised in #23053

@mnajdova mnajdova added status: waiting for maintainer These issues haven't been looked at yet by a maintainer discussion package: system Specific to @mui/system labels Oct 23, 2020
@oliviertassinari oliviertassinari removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 23, 2020
@yuchi
Copy link
Contributor

yuchi commented Oct 24, 2020

There’s no need for a pragma, you can release a Babel macro in a similar fashion to Styled Components’.

@re-thc
Copy link

re-thc commented Oct 26, 2020

How would Pragma work with the new automatic runtime? Has that been considered?

@mathieuhasum
Copy link

Looking forward to this feature!
As @yuchi mentioned, the Babel macro also seems to be a possible alternative.
Some reference I found for the implementation of the css prop in styled-components:
https://medium.com/styled-components/css-prop-support-for-create-react-app-37e8c5d96861
https://styled-components.com/docs/tooling#babel-macro

@yuchi
Copy link
Contributor

yuchi commented Nov 2, 2020

Please note that Styled's Babel Macro is a "bad macro", what I mean is that it's actually a Babel Plugin which is triggered file-wide at macro import (they release both the plug-in and the macro).
In this regard it is not an hygienic macro, it doesn't have a trigger reference: every occurrence of a css prop (by name! not by hygienic reference!) gets transpired.

@oliviertassinari oliviertassinari changed the title [system] Support sx on all primitive elements by introducing a jsx pragma [system] Support sx on all primitive elements Nov 20, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented May 26, 2021

There were also discussions about changing the prefered API from:

<Box component="footer" sx={{ display: "block" }}>
   My footer
</Box>

to

<x.footer sx={{ display: "block" }}>
  My footer
</x.footer>

inspired by https://xstyled.dev/. This is superior when you have long children. The closing tag is more helpful in the latter option (</x.footer> is more helpful than </Box>).

See #27207 (comment) for why it could be valuable for developers.

@siriwatknp
Copy link
Member

@oliviertassinari @mnajdova I would like to push this forward in the upcoming v5-beta. I think that this could make migration to v5 a lot easier. Take this as an example from notistack repo

// This is what normal v4 looks like

const styles = (theme: Theme) => createStyles({
    root: {
        // styles
    }
});

const SnackbarContent = forwardRef<HTMLDivElement, Props>(({ classes, className, ...props }, ref) => (
    <div ref={ref} className={clsx(classes.root, className)}  {...props} />
))

export default withStyles(styles)(SnackbarContent);

without primitives

import { styled } from '@material-ui/core/styles';

const Div = styled('div')(({ theme }) => ({
  // styles
}))

const SnackbarContent = forwardRef<HTMLDivElement, Props>(({ classes, className, ...props }, ref) => (
    <Div ref={ref} className={clsx(classes.root, className)}  {...props} />
))

export default SnackbarContent;

the issue with this approach is it make the structure of the project completely change, the component (Div) is created and usually there are more than 1 div which will be hard to migrate.

Primitives

// just an example of importing primitives
import { mui } from '@material-ui/core/primitives';

// remain the same
const styles = (theme: Theme) => createStyles({
    root: {
        // styles
    }
});

const SnackbarContent = forwardRef<HTMLDivElement, Props>(({ classes, className, ...props }, ref) => (
  <mui.div
    ref={ref}
    className={clsx(classes.root, className)}
    sx={theme => styles(theme).root}
    {...props}
  />
))

export default SnackbarContent;

The styles should remain the same and the html tag is replaced with primitives which make the code looks easier to review.

scottlamb added a commit to scottlamb/moonfire-nvr that referenced this issue Oct 3, 2022
I was struggling to upgrade the version of mui stuff (material and date
picker). I'm hoping getting rid of the deprecated stuff eases this a
bit.

I don't love that I can't just use sx on plain HTML stuff and have to
wrap it in Box, but oh well. Looks like I'm not alone, fwiw.
mui/material-ui#23220
@robphoenix
Copy link
Contributor

@mnajdova Hi, do you know if supporting sx on any html element is still being considered? Thanks :)

@mnajdova
Copy link
Member Author

mnajdova commented Feb 3, 2023

Hey @robphoenix we are not actively working on this at this moment. I believe we can kick off this effort again when we are closer to v6.

@robphoenix
Copy link
Contributor

Ok, grand, thanks for the update @mnajdova 🙇🏻

@robphoenix

This comment was marked as resolved.

@oliviertassinari

This comment was marked as resolved.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 14, 2024

@mui/pigment-css It looks like there is movement on this: https://twitter.com/PigmentCSS/status/1784828168748011578.

Can we systematically link PRs with issues? I can't find where this is happening, thanks.
Edit: same as mui/pigment-css#35?

@oliviertassinari oliviertassinari added the package: pigment-css Specific to @pigment-css/* label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: pigment-css Specific to @pigment-css/* package: system Specific to @mui/system
Projects
None yet
Development

No branches or pull requests

7 participants