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 back system props #24485

Merged
merged 18 commits into from
Jan 19, 2021
Merged

[Box] Add back system props #24485

merged 18 commits into from
Jan 19, 2021

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jan 18, 2021

This PR adds back the system props in the Box component. However ,the implementation is a bit different than the previous one. For it to work, I am just extending the value of the sx prop, by adding all system props at the start of the sx object. This means that the value of the sx prop will override these props consistently, which is what we want I believe.

TODO:

  • Update perf benchmarks
  • Add tests

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 18, 2021

@material-ui/core: parsed: +0.08% , gzip: +0.10%
@material-ui/system: parsed: +3.26% , gzip: +3.47%

Details of bundle changes

Generated by 🚫 dangerJS against 5f9a509

@mnajdova
Copy link
Member Author

mnajdova commented Jan 18, 2021

@mnajdova mnajdova marked this pull request as ready for review January 18, 2021 14:31
@mnajdova
Copy link
Member Author

After the PR is merged, I'll add more benchmarks for the system props too.

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.

Will we use the same approach to implement the props on the Typography or Grid component?

@mnajdova
Copy link
Member Author

Will we use the same approach to implement the props on the Typography or Grid component?

That's the plan, that's why I've added the extendSxProp utility, it should be pretty much straight forward to add it on any component we may need in the future.

@oliviertassinari oliviertassinari added the component: Box The React component. label Jan 18, 2021
@eps1lon eps1lon removed their request for review January 18, 2021 17:31
* @ignore - do not document.
*/
const Box = React.forwardRef(function Box(props, ref) {
const BoxInner = React.forwardRef((props, ref) => {
Copy link
Member

Choose a reason for hiding this comment

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

I assume we will be able to remove this inner component later down the road.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully yes. I had it inlined first, but I needed to add propTypes on it, that’s why I’ve extracted it..

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

Successfully merging this pull request may close these issues.

None yet

3 participants