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

[pigment-css] Add Box component #41451

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Conversation

brijeshb42
Copy link
Contributor

This is nothing but just a wrapper component with special handling for transformed sx prop.

@brijeshb42 brijeshb42 added the package: pigment-css Specific to @pigment-css/* label Mar 11, 2024
@mui-bot
Copy link

mui-bot commented Mar 11, 2024

Netlify deploy preview

https://deploy-preview-41451--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 3257ac3

@danilo-leal danilo-leal added the component: Box The React component. label Mar 11, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 12, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 12, 2024
const sxClass = typeof sx === 'string' ? sx : sx?.className;
const classes = [className, sxClass].filter(Boolean).join(' ');
// eslint-disable-next-line react/prop-types
const sxVars = sx && typeof sx !== 'string' ? sx?.vars : {};
Copy link
Member

Choose a reason for hiding this comment

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

How about?

Suggested change
const sxVars = sx && typeof sx !== 'string' ? sx?.vars : {};
// TODO remove, logic should be handled by the transpiler, needed anyway to support <div sx={{
const sxVars = sx && typeof sx !== 'string' ? sx?.vars : {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we'll be able to support <div sx={{}} /> in it's current form. It'll only be possible if there are no dynamic parts in the sx prop value, only then will we be able to replace it with className prop.
On the other hand, we could do something like <styled.div sx={{}} /> which could handle both static css as well as some dynamic values (limited by whatever is allowed in the sx prop).

Copy link
Member

Choose a reason for hiding this comment

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

It'll only be possible if there are no dynamic parts in the sx prop value

Meaning depending on props? I guess this is rare enough 😄? Most of the time I use sx I use static values.

we could do something like <styled.div sx={{}} />

Like @gregberge's https://xstyled.dev/. I don't know if we need to go as far as introducing a new API.

Copy link
Contributor Author

@brijeshb42 brijeshb42 Mar 13, 2024

Choose a reason for hiding this comment

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

In that case, the compiler can be updated to throw an error if it encounters any dynamic value. Otherwise, it'll just replace sx prop with a className prop

@oliviertassinari oliviertassinari changed the title [pigment][react] Add Box component [pigment-css] Add Box component Mar 13, 2024
This is nothing but just a wrapper component with special handling for
transformed sx prop.
Comment on lines +41 to +46
export type NoInfer<T> = [T][T extends any ? 0 : never];
type FastOmit<T extends object, U extends string | number | symbol> = {
[K in keyof T as K extends U ? never : K]: T[K];
};

export type Substitute<A extends object, B extends object> = FastOmit<A, keyof B> & B;
Copy link
Member

Choose a reason for hiding this comment

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

Let's find time to comment on the purpose of these types later 😅. I can imagine how hard to make the PolymorphicComponent work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Definitely. I hope this doesn't get more complex than this 🤣.

@brijeshb42 brijeshb42 merged commit 5c956ba into mui:master Mar 15, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Box The React component. package: pigment-css Specific to @pigment-css/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants