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] Add array support for sx prop #29297

Merged
merged 13 commits into from
Oct 27, 2021
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Oct 26, 2021

close #29274

I noticed one thing from #29198. Shouldn't the sx function callback give the whole props (theme is inside the props)?

current

<Button sx={theme => theme.typography.body} />

What I think

<Button sx={({ theme, ...otherProps }) => theme.typography.body} />
  • This is the same experience as writing styled
  • I guess there will be cases that other props are needed in sx

Decide to go with the current approach and see community feedback.

@mnajdova @hbjORbj


@siriwatknp siriwatknp added the package: system Specific to @mui/system label Oct 26, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Oct 26, 2021

Details of bundle changes

Generated by 🚫 dangerJS against c9ca06f

@mnajdova
Copy link
Member

<Button sx={({ theme, ...otherProps }) => theme.typography.body} />

This would be a breaking change, so we can't do it now. But we can wait to see if people will actually need it. I would say it's more common to add conditional styles based on where the component is being used, which means in the context of the parent component and developers anyway have access to those props :)

if (!isPlainObject(result)) {
return systemProps;
}
return { ...systemProps, ...result };
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we do deep merge?

Copy link
Member Author

@siriwatknp siriwatknp Oct 26, 2021

Choose a reason for hiding this comment

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

I don't think so. That will cause a lot more calculation. I think it is better to document that sx has more priority.

<Box p={2} sx={{ p: 3 }} /> // p: 3 wins

Also, the previous implementation used { ...systemProps, ...result } and there is no issue with it yet. So I don't think anyone will use the same property in both props & sx.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, let's leave it as is then :)

@siriwatknp
Copy link
Member Author

will run yarn proptypes && yarn docs:typescript:formatted && yarn docs:api once code review is done.

@siriwatknp siriwatknp requested a review from mnajdova October 26, 2021 10:44
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks good

@siriwatknp siriwatknp force-pushed the feat/sx-array branch 2 times, most recently from 41cfc61 to bde7da6 Compare October 27, 2021 03:10
Copy link
Member

@hbjORbj hbjORbj left a comment

Choose a reason for hiding this comment

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

:)

@mnajdova mnajdova merged commit d481d1a into mui:master Oct 27, 2021
@siriwatknp siriwatknp mentioned this pull request Nov 8, 2021
1 task
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow array as sx value to have conditional 'sub-classes'
4 participants