nusmodifications / nusmods

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

Grey Out Deprecated Courses In PreReq Tree #3647

Closed EssWhyy closed 4 months ago

EssWhyy commented 6 months ago

Context

Implements #3473

Implementation

Connects ModuleTree.tsx to the Redux Store, and uses the getModuleCondensed() function to check if the module code in each node points to a valid module in the current module bank. If the module has been deprecated, we remove the hoverable color-${layer} classname tag from the module node. The node will be greyed out and no longer appear hoverable as shown below

UI Example:

deprecated courses

Other Information

vercel[bot] commented 6 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 Mar 4, 2024 9:39am
nusmods-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 4, 2024 9:39am
vercel[bot] commented 6 months ago

@EssWhyy is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel.

@nusmodifications first needs to authorize it.

codecov[bot] commented 4 months ago

Codecov Report

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

Project coverage is 53.71%. Comparing base (1ec2b2b) to head (e156a0b).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3647 +/- ## ========================================== + Coverage 53.69% 53.71% +0.02% ========================================== Files 272 272 Lines 5956 5959 +3 Branches 1417 1418 +1 ========================================== + Hits 3198 3201 +3 Misses 2758 2758 ```

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

yimqiy commented 3 months ago

Ooh just saw this :) thanks for implementing!

Just a note - pre-reqs that are not individual courses to begin with are also greyed out, like the one below: image

kokrui commented 3 months ago

@yimqiy thanks for the comment! I do think it makes sense to have different behaviour (i.e. don't just grey them out) for these non-course pre-req tree nodes -- let's track this as an issue