nusmodifications / nusmods

🏫 Official course planning platform for National University of Singapore.
https://nusmods.com
MIT License
590 stars 319 forks source link

Reduce SASS Deprecation Warnings #3823

Closed kaze-droid closed 2 months ago

kaze-droid commented 2 months ago

Context

Partial Fix for Issue #3819

Implementation

Most SASS warnings were fixed by upgrading the packages:

yarn upgrade sass bootstrap @material/theme @material/ripple @material/fab

To temporarily fix the warning on passing percentage units to the global abs() function, I created a custom SCSS solution that defines an abs() function in src/styles/scss/bootstrap/function-overrides.scss that internally uses math.abs() and import it before the Bootstrap SCSS files

For the warning on using / for division, I used the automatic Sass migrator to refactor the code

npx sass-migrator division '**/*.scss'

However, this approach has limitations. It incorrectly uses math.div instead of list.slash in node_modules/bootstrap/scss/mixins/_forms.scss. Additionally, running the migrator with the --pessimistic flag resulted in numerous false negatives. For more details, sass-migrator issues

Other Information

Reduced SASS deprecation warnings Image of reduced SASS deprecation warnings

A complete and permanent solution to these warnings will require migrating to BS5

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 5:40pm
nusmods-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 19, 2024 5:40pm
vercel[bot] commented 2 months ago

@kaze-droid is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 54.59%. Comparing base (2d4743d) to head (ef9a28d). Report is 31 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3823 +/- ## ========================================== + Coverage 53.87% 54.59% +0.72% ========================================== Files 274 274 Lines 6032 6066 +34 Branches 1449 1453 +4 ========================================== + Hits 3250 3312 +62 + Misses 2782 2754 -28 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ravern commented 2 months ago

Thanks for your contribution!

I think the sass-migrator work isn't committed in this branch, can I confirm this is intentional, i.e., it doesn't work and won't be a part of this PR?

kaze-droid commented 2 months ago

Oops I initially ran sass-migrator locally, which only updated the files in my local node_modules directory. I've just added a new script in package.json to run sass-migrate before the build process.

zwliew commented 2 months ago

Oops I initially ran sass-migrator locally, which only updated the files in my local node_modules directory. I've just added a new script in package.json to run sass-migrate before the build process.

I think there has been a mistake. sass-migrator should only need to be run once on the SCSS files in our repo (excluding the node_modules directory) and should not be committed to the repo. So, please revert your latest change.

For any dependencies (in node_modules) which have SASS deprecation warnings, we should probably update those dependencies, but if it is too much work to do so, we can update them in a future PR.

The clarification/question Ravern was posing is: does running sass-migrator not affect any of our SASS files, meaning that it only affects files in node_modules?

kaze-droid commented 2 months ago

Apologies for the confusion. I've reverted the changes as requested. Running sass-migrator only impacted files within node_modules, specifically with deprecation warnings originating from @material/theme. I attempted to update it, but the latest version that still supports BS4 doesn't resolve those warnings.