-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Alert] Add support for CSS vars #32624
Conversation
@material-ui/core: parsed: +0.55% , gzip: +0.29% |
The Alert needs to wait for a discussion about how to deal with |
I just changed the needed, and was waiting for the info about it |
@haneenmahd we have a resolution for the problem in #32049 (comment) Feel free to continue with the effort. |
errorColor: string; | ||
infoColor: string; | ||
successColor: string; | ||
warningColor: string; | ||
errorFilledBg: string; | ||
infoFilledBg: string; | ||
successFilledBg: string; | ||
warningFilledBg: string; | ||
errorStandardBg: string; | ||
infoStandardBg: string; | ||
successStandardBg: string; | ||
warningStandardBg: string; | ||
errorIconColor: string; | ||
infoIconColor: string; | ||
successIconColor: string; | ||
warningIconColor: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnajdova I could not see other ways to reduce the number of tokens if we want to keep the same behavior. 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be tricky if people add custom colors here :\ Should we maybe create these based on the PaletteOptions type?
There was a problem hiding this comment.
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 should go that far for now. There is a trade-off because some people might not want the autogenerated variables (increase in bundle size) and we need to create more APIs to let them exclude things that they don't want.
At this point, I think we can leave the new custom palette effort to the developers and make sure that the docs are clear and comprehensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Last PR for CSS variables support. I'm pumped!
sorry, for asking but what exactly needed to be changed RN? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, left one comment on the types.
Add support for CSS variables for the component
Alert
(#32049).guide.