fix(material/schematics): Create a schematic to add the base theme dimension #27964
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewseguin
approved these changes
Oct 19, 2023
There was a problem hiding this comment.
All looks good to me - thanks for making the functions clear to understand. It seems there's a fair amount of utility functions that could be useful to other migrations in the future - would be nice if they were in a shared folder or otherwise we should remember to refactor that another time.
One question - would it be better to not define dummy-theme so that it causes a compilation error? I'm worried users will not realize we added this and it'l silently work.
src/material/schematics/ng-update/migrations/theme-base-v17/migration.ts
Outdated
Show resolved
Hide resolved
mmalerba
force-pushed
the
base-schematic
branch
2 times, most recently
from
f9ccef0
to
4bbf77c
Compare
…mension As of v17, users need to include the new "base" theme dimension for the components they use. For users that are using the "theme" mixins now, the base dimension will be pulled in automatically. However, for users using the "color", "typography", and "density" mixins only, they will need to include the "base" mixin as well. This schematic works by scanning all of the app's Sass to determine which components had their "color", "typography", or "density" mixins included, but did *not* have their "theme" mixin included. It then locates all of the calls to "mat.core()" and inserts calls to the "base" mixins for the identified compoennts, immediately following. It makes sense to use "mat.core()" as a signal for when to insert them, because the "base" mixins should be used once-per-app, like "mat.core()". We can't guarantee that a "$theme" will be available at the insertion point, so we create a "$dummy-theme" to pass to the base mixins. We also insert a comment above the new mixins explaining why they were added and a TODO to clean up the "$dummy-theme". Once we have the documentation updated to cover the base dimension, we should follow-up by linking to it from the inserted comment.
mmalerba
force-pushed
the
base-schematic
branch
from
4bbf77c
to
9311d67
Compare
crisbeto
reviewed
Oct 20, 2023
src/material/schematics/ng-update/migrations/theme-base-v17/migration.ts
Show resolved
Hide resolved
crisbeto
approved these changes
Oct 20, 2023
mmalerba
added a commit
that referenced
this pull request
Oct 20, 2023
…mension (#27964) As of v17, users need to include the new "base" theme dimension for the components they use. For users that are using the "theme" mixins now, the base dimension will be pulled in automatically. However, for users using the "color", "typography", and "density" mixins only, they will need to include the "base" mixin as well. This schematic works by scanning all of the app's Sass to determine which components had their "color", "typography", or "density" mixins included, but did *not* have their "theme" mixin included. It then locates all of the calls to "mat.core()" and inserts calls to the "base" mixins for the identified compoennts, immediately following. It makes sense to use "mat.core()" as a signal for when to insert them, because the "base" mixins should be used once-per-app, like "mat.core()". We can't guarantee that a "$theme" will be available at the insertion point, so we create a "$dummy-theme" to pass to the base mixins. We also insert a comment above the new mixins explaining why they were added and a TODO to clean up the "$dummy-theme". Once we have the documentation updated to cover the base dimension, we should follow-up by linking to it from the inserted comment. (cherry picked from commit c92fdad)
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
As of v17, users need to include the new "base" theme dimension for the components they use. For users that are using the "theme" mixins now, the base dimension will be pulled in automatically. However, for users using the "color", "typography", and "density" mixins only, they will need to include the "base" mixin as well.
This schematic works by scanning all of the app's Sass to determine which components had their "color", "typography", or "density" mixins included, but did not have their "theme" mixin included. It then locates all of the calls to "mat.core()" and inserts calls to the "base" mixins for the identified compoennts, immediately following. It makes sense to use "mat.core()" as a signal for when to insert them, because the "base" mixins should be used once-per-app, like "mat.core()". We can't guarantee that a "$theme" will be available at the insertion point, so we create a "$dummy-theme" to pass to the base mixins.
We also insert a comment above the new mixins explaining why they were added and a TODO to clean up the "$dummy-theme". Once we have the documentation updated to cover the base dimension, we should follow-up by linking to it from the inserted comment.