feat: Enable flexible number of colors for themes #3482
Conversation
- instead of fixing the number of colors for a theme to 8 colors, now we are able to support a variable number of colors - add a new theme, 'Tequila' with 10 colors as Proof of Concept
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@joeng03 is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel. @nusmodifications first needs to authorize it. |
…ps://github.com/joeng03/nusmods into joeng03/support-flexible-num-of-colors-in-theme
… modules after selectTheme
…g03/support-flexible-num-of-colors-in-theme
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3482 +/- ##
==========================================
- Coverage 53.44% 53.34% -0.10%
==========================================
Files 271 271
Lines 5862 5882 +20
Branches 1396 1397 +1
==========================================
+ Hits 3133 3138 +5
- Misses 2729 2744 +15
☔ View full report in Codecov by Sentry. |
|
Hi, thanks for working on this! Is this PR still WIP as the description states or is it ready for review? |
|
It is ready for review! |
| onSelectTheme={() => { | ||
| props.selectTheme(theme); | ||
| props.reassignAllModulesColor(theme.numOfColors); | ||
| }} |
There was a problem hiding this comment.
This has the disadvantage of updating the UI twice because we are dispatching two actions here. To fix this, you could remove the reassignAllModulesColor action, and change the timetables reducer to react to the SELECT_THEME action.
|
|
||
| return { | ||
| ...state, | ||
| id: newTheme, | ||
| id: newTheme.id, | ||
| numOfColors: newTheme.numOfColors, |
There was a problem hiding this comment.
Since this affects local storage schema, we should be more careful here. In the long run, I don't think we'll want to store numOfColors in ThemeState as it only contains user customizable data (i.e. current theme ID, should show title, orientation of timetable).
Since numOfColors is a static property of a theme, instead of storing it in Redux, we should store it in a predefined readonly map like the one that you've defined in themes.json. This has the added benefit of you no longer having to pass numOfColors around everywhere as you can just read it from the map.
That said, if/when we add user-defined themes, they should be stored in Redux, but likely in a different place instead of ThemeState.
website/src/reducers/timetables.ts
Outdated
| @@ -148,6 +149,19 @@ function semColors(state: ColorMapping = defaultSemColorMap, action: Actions): C | |||
| } | |||
| } | |||
|
|
|||
| function recolor(state: SemesterColorMap, numOfColors: number): SemesterColorMap { | |||
There was a problem hiding this comment.
Minor thought: This is a fine approach, but I wonder if it'd be better to remove this function and leave color indices as is. Instead, we could modulo the colorIndex when rendering colors. I doubt it makes any difference though.
There was a problem hiding this comment.
The reason I used an action to reassign the colors is that there are too many places where we have to modulo the colorIndex when rendering colors, which might make code changes hard to keep track of.
|
The implementation looks sound but I have a few concerns mainly regarding (1) storing |
|
Thanks for taking time to review my PR :)
Would it be better if we combine steps 1 and 2 in this PR? |
Context
#3443
Implementation
One issue I was facing was that when shifting from themes with higher number of colors to themes with lower number of colors, some courses would get rendered as white color as their colorIndex exceeds the numOfColors of the current theme. One way to solve this would be to dispatch a new action to set the colorIndex of all courses to colorIndex % numOfColors.
Other Information