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

Ban sx atrribute in all pigment components and skip compilation #45085

Closed
povilass opened this issue Jan 22, 2025 · 5 comments
Closed

Ban sx atrribute in all pigment components and skip compilation #45085

povilass opened this issue Jan 22, 2025 · 5 comments
Assignees
Labels
package: system Specific to @mui/system support: question Community support but can be turned into an improvement

Comments

@povilass
Copy link
Contributor

povilass commented Jan 22, 2025

Summary

Migrating from v4 -> v5 has been a rollercoaster migrating makestyles to styled that introduced SX property and made life easier.

Migrating from v5 -> v6 is still unclear how to workaround some cases of the dynamic context of styling and introduced unwanted experiences for developers (for middle, junior level developers mostly) on how to work with SX property. Most cases can be solved using component + styled + classes combination over SX which is better and clearer how to do it.

Examples

No response

Motivation

It would be nice to disable this feature if possible and choose a proper styling method for consistency throughout the application. I do not think, that SX is the future of styling and probably it will shift in v7 or v8 material release again.

Note: disabled, only for pigment components, not emotion for easy migration.

Search keywords: sx, not the future

@povilass povilass added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 22, 2025
@zannager zannager added the package: system Specific to @mui/system label Jan 22, 2025
@siriwatknp
Copy link
Member

some cases of the dynamic context of styling and introduced unwanted experiences for developers (for middle, junior level developers mostly) on how to work with SX property

Can you give some example of these cases. We could put it on the docs to benefit other people too.

@siriwatknp siriwatknp added 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 Jan 27, 2025
@brijeshb42
Copy link
Contributor

You can do it even now without any changes in the library.

In Pigment's config, pass transformSx: false and also don't augment TS to support the sx attribute.
This'll also save some time on the sx transform that we have to do.

@povilass
Copy link
Contributor Author

povilass commented Jan 28, 2025

@siriwatknp The reason is that junior and middle developers do not read docs. They usually work with the try-fail method.

For example, in the map function,n you will get an error for no reason on SX because of compile time

<div>
  {[0, 1, 2, 3].map((item) => 
    <Fragment key={item}>
      <Stack spacing={2} sx={{color: 'red'}}>
        <Item>Item 1</Item>
        <Item>Item 2</Item>
        <Item>Item 3</Item>
      </Stack>
    </Fragment>)
   }
</div>

But if you use styled it gives the same effect and almost generates the same styles in the CSS files without any penalty than using it.

const Root = styled('div')({
  '& .element': {color: 'red'}
})

<Root>
  {[0, 1, 2, 3].map((item) => 
    <Fragment key={item}>
      <Stack spacing={2} className="element">
        <Item>Item 1</Item>
        <Item>Item 2</Item>
        <Item>Item 3</Item>
      </Stack>
    </Fragment>)
   }
</Root>

@brijeshb42 yes you can but the is a problem with another task with typography in v7 color property will be eliminated and we can only apply color through sx.

Will not be supported <Typography color="text.primary" />
Will supported only with sx property <Typography sx={{ color : "text.primary"}} />

But this is for another discussion.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Jan 28, 2025
@siriwatknp
Copy link
Member

But this is for another discussion.

Then please open another issue since you can disable sx transformation from the config as @brijeshb42 pointed out.

@siriwatknp siriwatknp added support: question Community support but can be turned into an improvement and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 31, 2025
Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@povilass How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system support: question Community support but can be turned into an improvement
Projects
None yet
Development

No branches or pull requests

4 participants