feat: Enable flexible number of colors for themes

[joeng03]

• 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

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

[vercel[bot]]

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	Nov 26, 2024 11:10am
nusmods-website	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 26, 2024 11:10am

[vercel[bot]]

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

@nusmodifications first needs to authorize it.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 43.90244% with 23 lines in your changes missing coverage. Please review.

Project coverage is 54.58%. Comparing base (988c6fd) to head (0699b0a). Report is 21 commits behind head on master.

Files with missing lines	Patch %	Lines
website/src/reducers/timetables.ts	0.00%	10 Missing :warning:
website/src/actions/timetables.ts	16.66%	5 Missing :warning:
website/src/views/settings/SettingsContainer.tsx	0.00%	3 Missing :warning:
website/src/entry/export/TimetableOnly.tsx	0.00%	2 Missing :warning:
website/src/views/venues/VenueDetails.tsx	0.00%	2 Missing :warning:
website/src/views/settings/ThemeOption.tsx	0.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3482      +/-   ##
==========================================
+ Coverage   54.52%   54.58%   +0.05%     
==========================================
  Files         274      274              
  Lines        6076     6119      +43     
  Branches     1455     1461       +6     
==========================================
+ Hits         3313     3340      +27     
- Misses       2763     2779      +16     

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

[zwliew]

Hi, thanks for working on this! Is this PR still WIP as the description states or is it ready for review?

[joeng03]

It is ready for review!

[zwliew]

The implementation looks sound but I have a few concerns mainly regarding (1) storing numOfColors in Redux and (2) dispatching multiple actions successively. Please let me know what you think :)

[joeng03]

Thanks for taking time to review my PR :)

1. Storing numOfColor in Redux is a temporary step as we will store all themes ( default and user defined) in a new Redux state, which uses an array of hexcodes to track all colors of a theme, then use an id in ThemeState to reference the current theme.

2. I agree that dispatching multiple actions is suboptimal, and we can refactor the code to reassign the colorIndex in the selectTheme instead. However, since we no longer need numOfColors in the next step, it may be more efficient to move straight to the next step ( use an array of hexcodes instead of numOfColors to render colors of a theme) since we will no longer need the reassignAllColors action anymore.

Would it be better if we combine steps 1 and 2 in this PR?

[zwliew]

Thanks for taking time to review my PR :)

1. Storing numOfColor in Redux is a temporary step as we will store all themes ( default and user defined) in a new Redux state, which uses an array of hexcodes to track all colors of a theme, then use an id in ThemeState to reference the current theme.
2. I agree that dispatching multiple actions is suboptimal, and we can refactor the code to reassign the colorIndex in the selectTheme instead. However, since we no longer need numOfColors in the next step, it may be more efficient to move straight to the next step ( use an array of hexcodes instead of numOfColors to render colors of a theme) since we will no longer need the reassignAllColors action anymore.

Would it be better if we combine steps 1 and 2 in this PR?

Really sorry for the late review! I agree with what you said -- it would be more straightforward to combine steps 1 and 2. Would you be interested in continuing with this PR? I'd like to try to get a solution merged by the end of the holidays.

[joeng03]

Yep, will be working on it in December since I’m having finals now

[zwliew]

That's great to hear :) Best of luck for finals!