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] Fix sx prop types when CSS variables are used with nested selectors #31163

Merged
merged 4 commits into from
Feb 23, 2022

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Feb 21, 2022

I noticed this in #31138. Basically, if the CSS variables are used together with some nested selectors in the sx prop, it breaks the types. I've added tests and update the types, see #31163 (comment).

I had to remove two @ts-expect-errors, not ideal, but I think fixing the problem with the CSS variables used together with the nested selectors is more important.

@mui-bot
Copy link

mui-bot commented Feb 21, 2022

No bundle size changes

Generated by 🚫 dangerJS against b9a4ff6

@@ -48,8 +55,7 @@ export type SystemCssProperties<Theme extends object = {}> = {
export type SystemStyleObject<Theme extends object = {}> =
| SystemCssProperties<Theme>
| CSSPseudoSelectorProps<Theme>
| CSSSelectorObject<Theme>
| { [cssVariable: string]: string | number }
Copy link
Member Author

Choose a reason for hiding this comment

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

[cssVariable: string] is basically the same as [cssSelector: string] defined in CSSSelectorObject, so whichever will be mapped first, will invalidate the other, hence the CSS variables cannot be defined in the same object as nested selectors (see the tests)

@mnajdova mnajdova added package: system Specific to @mui/system typescript labels Feb 21, 2022
@mnajdova mnajdova marked this pull request as ready for review February 21, 2022 14:35
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Awesome

Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

I had to remove two @ts-expect-errors, not ideal, but I think fixing the problem with the CSS variables used together with the nested selectors is more important.

We need to loosen the type strictness anyway, so I wouldn't worry about this much. I can't think of a solution better than you proposed.

@c8fea132-8c53-40e7-b9f7-b3d78dc540da

This comment was marked as abuse.

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 typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants