Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/core): migrate to the Sass module system #21204
feat(material/core): migrate to the Sass module system #21204
Changes from all commits
84f273f
d94af9c
d43cb99
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
What happens if we remove this top line? My guess as to what's happening here is that it's attempting to forward
cdk-optionally-nest-content
, but that then gets hidden on the following line.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.
If we removed this line, somebody using
@import
will get global mixins calleda11y
andhigh-contrast
.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 tried something like this locally:
Running
sass ./leaf.scss
errors here withError: Undefined mixin.
since the mixin is aliased as part of the@forward
. Includingnum-one()
instead works fine. I think that without explicitly forwarding the mixins, they aren't surfaced?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.
This could be in a follow-up PR, but I'd like to rename this mixin to something like
visually-hidden
since my original name here (cdk-a11y
) was really way too vague (I had thought we'd add more stuff to it, but that ended up not happening).(with the old name deprecated for backwards compatibility, of course)
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.
It can be done in a follow-up. I'm trying to keep this PR only to the migration itself.
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.
For the cdk entry points with Sass (a11y, overlay, textfield), I think we need to also include these
.import.scss
files in the root of the cdk package. We currently copy each partial to the root, so we need to keep backwards compatibility there.Separately, I think we should introduce a
_cdk.scss
file in the root of the package that should become the primary entry point into cdk styles to let people do stuff likeThere 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'm not sure I get this part:
My understanding is that all the Sass files get copied to the same place in the
dist
so as long as the.import
file is next to the base .scss file, everything should work like before. Here's what the release output forcdk/overlay
looks like:As for the proposal to add a
cdk.scss
, I agree but I'd rather do it in a separate PR so this one only has the migration-specific changes.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 just tried this locally and everything does work as expected, I just don't understand why it works. We copy
_a11y.scss
,_overlay.scss
, and_text-field.scss
into the root of the package. Since the names of the mixins changed (e.g.cdk-overlay
tooverlay
), I thought that the following would fail:But it actually works just fine.
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 doesn't sound right, you might have something cached which is preventing it from failing. I'll look into copying the
.import
files too.