-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(material-experimental/mdc-core): add missing color, density, typography mixins #24063
Conversation
@use './option/option-theme'; | ||
@use './option/optgroup-theme'; | ||
@use './elevation'; | ||
|
||
|
||
@mixin color($config-or-theme) { |
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.
Is this supposed to be the equivalent of the non-MDC core theme? If yes, it needs some more styles like the ripple and pseudo checkbox.
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.
currently we don't have an MDC-based ripple or pseudo checkbox. I think this should be everything for now
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.
Couldn't we include the non-MDC ripple here and use this mixin instead of the non-MDC one? Currently we include a bunch of unnecessary styles in the prebuilt themes, because we go through the non-MDC core
mixin which contains the typographies of all non-MDC components. If you're blocked on this, it could be done in a follow-up.
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 that would be a good idea, usually people include both the old mixins and new mixins while they're migrating, so if we did that we'd wind up with double the bytes
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.
That's basically what we've been doing with the TS code though.
cfcdb93
to
e15b582
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.