Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

refactor: migrate to the Sass module system #5453

Merged
merged 2 commits into from
Jan 14, 2020

Conversation

jathak
Copy link
Contributor

@jathak jathak commented Jan 10, 2020

This PR was generated by running this script based on the Sass migrator. It migrates all components to the Sass module system, removing manual mdc-$component prefixes and implicit dependencies between files (though still leaving both intact for users still using @import).

High-level overview of the changes made here:

  • Prefixes (e.g. mdc-button-) for each component have been removed from all variables, functions, and mixins within the code. Prefixes on CSS selectors are unaffected.
  • When depending on one of a component's files via @use, only members directly declared in that file are available. The mdc-$component.scss files should only expose CSS, not Sass members.
  • Import-only files have been generated that add back all of the prefixes and explicitly forward all members that were implicitly available via @import previously, so existing users that still use @import shouldn't break as a result of these changes. These files will then be used by the migrator when those users later migrate to the module system themselves.
  • Minor breaking change: Four variables and a mixin in mdc-textfield were renamed to use a mdc-text-field- prefix when depended on via @import. None of the variables are used within Google and the mixin (formerly mdc-required-text-field-label-asterisk_, now required-label-asterisk_) is used only once by MWC.

@jathak
Copy link
Contributor Author

jathak commented Jan 10, 2020

One other note about the script used here: while the underlying Sass migrator is fairly robust, the script itself is fragile, as preserving the proper prefixes for some of the more complex components (e.g. mdc-ripple, mdc-select, mdc-textfield) required a bunch of sed transformations, some of which reference specific lines to change. We can reuse the script if there are enough changes to master before this is submitted to make a merge difficult, but future runs may require tweaks to it.

@codecov-io
Copy link

codecov-io commented Jan 10, 2020

Codecov Report

Merging #5453 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5453      +/-   ##
==========================================
- Coverage   97.06%   97.04%   -0.02%     
==========================================
  Files         164      164              
  Lines        6266     6266              
  Branches      824      782      -42     
==========================================
- Hits         6082     6081       -1     
- Misses        184      185       +1
Impacted Files Coverage Δ
testing/dom/events.ts 71.42% <0%> (-28.58%) ⬇️
packages/mdc-auto-init/index.ts 96% <0%> (+4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 981ec9b...a19bf66. Read the comment docs.

@jathak jathak force-pushed the module-system-migration branch 2 times, most recently from 62c6d7c to 5593f9f Compare January 10, 2020 22:07
@jathak
Copy link
Contributor Author

jathak commented Jan 10, 2020

Okay, the lint issue was that stylelint-scss was out of date. The unit tests seem to be failing because of an issue with Chrome

@jathak
Copy link
Contributor Author

jathak commented Jan 11, 2020

cl/289132951

@jathak jathak force-pushed the module-system-migration branch from 4142d1a to a19bf66 Compare January 13, 2020 20:45
Copy link
Collaborator

@abhiomkar abhiomkar left a comment

Choose a reason for hiding this comment

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

🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants