Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Themes should support more than 8 colours #3443

Open
zwliew opened this issue Jun 24, 2023 · 5 comments
Open

Themes should support more than 8 colours #3443

zwliew opened this issue Jun 24, 2023 · 5 comments

Comments

@zwliew
Copy link
Member

zwliew commented Jun 24, 2023

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. :')

@zwliew zwliew changed the title Support for more than 8 colours per theme Themes do not support more than 8 colours Jun 24, 2023
@zwliew zwliew changed the title Themes do not support more than 8 colours Themes should support more than 8 colours Jun 24, 2023
@joeng03
Copy link
Contributor

joeng03 commented Jul 28, 2023

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
Copy link
Member Author

zwliew commented Aug 1, 2023

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
Copy link
Contributor

joeng03 commented Aug 3, 2023

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
Copy link
Member Author

zwliew commented Aug 3, 2023

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
Copy link
Member

li-kai commented Mar 27, 2024

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants