nusmodifications / nusmods

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

Themes should support more than 8 colours #3443

Open zwliew opened 1 year ago

zwliew commented 1 year ago

Is your feature request related to a problem? Please describe.

Each theme only supports 8 colours. This is an issue for users with more than 8 courses on their timetables as some courses will have duplicate colours.

Describe the solution you'd like

Themes should comprise more than 8 colours.

Describe alternatives you've considered

Possible extension is to support custom themes where users can add their own colours. Existing themes can be inherited/extended into custom themes.

Additional context

I just wish my timetable will stop gaslighting me with duplicate colours. :')

joeng03 commented 1 year ago

Hi Zhao Wei, I am new here and really interested in taking this up!

Themes can be shared with others by encoding them in hexadecimal, and through decoding the hexadecimal, we can inherit/extend existing themes. The component for making custom themes should be a relatively simple one, allowing imports while integrating a ColorPicker component from a library.

The main challenge here is that we might have to rewrite how we render the color of the lesson blocks. Currently, it is purely set by .scss and the color cannot be modified/added. What I have in mind is to either:

i. rewrite existing code as a theme array that contains theme items. Stop using .scss for lesson block coloring and instead use js objects containing the hex string. (potentially painful) ii. Keep the default themes in .scss, convert the theme array from JS to .scss so we can keep the current color rendering method for lesson blocks.

Please share your thoughts and let me know if you have any other suggestions!

zwliew commented 1 year ago

Thanks for giving this some thought :) Suggestion 2 sounds pretty roundabout, but suggestion 1 sounds good albeit time-consuming.

Since this is a large change, we'd ideally have several incremental PRs to get the theme picker to a better state. Off the top of my head, something like this makes sense:

  1. Add/modify a theme with ~10 colors and implement UI support for it (i.e. an MVP to resolve this issue)
  2. Refactor the codebase to represent themes differently (i.e. in JS)
  3. Implement theme extensions

As a start, it would be good to add a new built-in theme with ~10 colors (or modifying an existing theme) and implement UI support to display all its colors properly. What do you think?

joeng03 commented 1 year ago

Thanks, I think this makes a lot of sense!

I just made a PoC for step 1. A new theme 'Tequila' with 10 colors have been added to the default themes.

Screenshot 2023-08-03 124401

One bug 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 an action to set the colorIndex of all courses to colorIndex % numOfColors.

The current Theme object looks like this:

export type Theme = {
  readonly id: ThemeId;
  readonly name: string;
  readonly numOfColors: number;
};

and I plan to convert it into something like this in step 2:

export type Theme = {
  readonly id: ThemeId;
  readonly name: string;
  readonly colors: string[];
};

Please add me as a contributor so that I can create a PR :))

zwliew commented 1 year ago

Please add me as a contributor so that I can create a PR :))

You should be able to fork the project, push your changes onto the fork, then create a PR from your fork into master.

li-kai commented 5 months ago

Small thing but make sure to test the colors work well in both light/dark mode and aren't too hard to read. Monokai already suffers from this somewhat (But that's another story).