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

Add new theming system #2833

Merged
merged 18 commits into from May 10, 2024
Merged

Add new theming system #2833

merged 18 commits into from May 10, 2024

Conversation

joeyballentine
Copy link
Member

@joeyballentine joeyballentine commented May 3, 2024

  • Renamed the old "theme" to "color mode" (light/dark mode)
  • New "theme" option that for now only has two options, "default" and "charcoal". I plan on adding more themes in future PRs.
  • Tidied up colors.scss a lot and made a lot of the color setting more automatic.
  • Fixed a couple light mode issues

Right now themes are just based on the 0-900 token-based color scheming, but there's also no reason you couldn't add overrides for specific things in the [data={theme}] attributes if you wanted for a theme.

issues:

  • The control buttons and the minimap currently are not themed as the colors are hardcoded to be blue. I'll tackle that later.

Example of new theme:
image
image

@RunDevelopment
Copy link
Member

Not a fan of the new system from 2 angles:

  1. I don't like this theming system.
    • You forced all themes to have both a light and dark mode.
    • The theme selection is overly complex. It could have just been System, Light, Dark, Charcoal Light, Charcoal Dark.
  2. I don't see why we need this. Designing new UI elements and changing existing ones is kinda difficult already, because we have to test everything in 2 themes. What do we gain from making it even harder?

@joeyballentine
Copy link
Member Author

The reason i can't just do that is because chakra has a lot of internal things that rely on the data-theme setting just being "dark" or "light". If i change that to "charcoal-light" it all breaks. though we could probably do something with the way that setting dropdown works to set the separate settings depending on which mode the theme is, so maybe that isn't really a concern.

Anyway, I thought that this would just be a bit of a convenience thing to get both color modes per theme, but i can redo this in way where each theme is individually crafted all the time with lots of code duplication if you prefer it that way.

@joeyballentine
Copy link
Member Author

Also, in terms of making it difficult/harder, due to the way I've set this up right now we would not need to test everything in every theme. Which colors are used for what is only determined by the dark and light modes. The colors themselves are chosen by the theme. So long as you use a standard array of shades for each theme, you shouldn't have any issues.

Doing what you suggest would turn it into the concern you have about overcomplicating everything, since there would thereby be no standard for what colors map to what.

@RunDevelopment
Copy link
Member

Also, in terms of making it difficult/harder, due to the way I've set this up right now we would not need to test everything in every theme. Which colors are used for what is only determined by the dark and light modes. The colors themselves are chosen by the theme. So long as you use a standard array of shades for each theme, you shouldn't have any issues.

Uhmmm, is that really the extent of the new system? I thought that you just made a super simple system to get started. If the theming system only allows themes to define the shades of gray used, then all themes are essentially just gray with a slight tint towards some color. So yes, this does solve the problem of having to design for many themes, but only because there will be only one theme, just with a different tint each time.

This theming system wouldn't even allow themes to define accent colors, right?

@joeyballentine
Copy link
Member Author

joeyballentine commented May 3, 2024

You can add individual color overrides to each theme, sure (though, I'm not actually sure if they would override the dark/light mode ones... I thought they would but I haven't tested it yet). But right now the extent of this is just having tinted gray, yes.

I would like to build out a more extensive theming system in the future, but right now this simple one should allow us to include basic themes for those that want to use them without much hassle.

@joeyballentine
Copy link
Member Author

If i were to combine the themes and just have Default (Dark), Default (Light), Charcoal (Dark), and Charcoal (Light), how would the "system" setting work? It's not a theme, so what theme would it pick for light mode or dark mode? Default? What if i want it to follow my system theme but but i want to use charcoal? The way I have it now would allow that

@joeyballentine
Copy link
Member Author

Actually, we should just copy how vscode handles it: https://code.visualstudio.com/docs/getstarted/themes#_automatically-switch-based-on-os-color-scheme

@joeyballentine
Copy link
Member Author

This theming system actually does allow overrides per theme, so that's cool

@joeyballentine
Copy link
Member Author

So, I think all the concerns from this are lifted now. Dark and light themes are selected separately. I spent some time experimenting with what is possible under this system by making a few more themes. Overriding more than just the "gray" colors is possible after all, which led to me being able to make some interesting themes.

Obviously it will require a larger refactor to make this more powerful, but I think this is a good starting point. Let me know if you think we should change or remove any of the themes I made.

Also, we could hide this under the experimental features toggle if we don't want to expose this by default quite yet

@RunDevelopment
Copy link
Member

Actually, we should just copy how vscode handles it: https://code.visualstudio.com/docs/getstarted/themes#_automatically-switch-based-on-os-color-scheme

No, we should not. These settings are useful for when you share the same VSCode settings across multiples machines/people. Nobody uses them when just setting the theme of their machine.

You completely overengineered this. The UX is also pretty bad too. Try explaining to a new user what all 3 settings do, when they just wanted to change the theme. And only one of those 2 theme settings actually does anything at a time.

A simple dropdown would have been enough. You know, kinda like the one VSCode has:

image

@joeyballentine
Copy link
Member Author

Ok, I'll just remove the "system" color mode option then, since that would not work with this

@RunDevelopment
Copy link
Member

Sounds good. Be sure to migrate settings though.

@joeyballentine
Copy link
Member Author

I was being sarcastic but ok, sure.

@joeyballentine
Copy link
Member Author

How exactly do I migrate settings under the new settings system?

src/renderer/components/SettingsModal.tsx Outdated Show resolved Hide resolved
src/renderer/global.scss Show resolved Hide resolved
src/renderer/components/SettingsModal.tsx Outdated Show resolved Hide resolved
src/renderer/contexts/SettingsContext.tsx Outdated Show resolved Hide resolved
src/renderer/contexts/SettingsContext.tsx Outdated Show resolved Hide resolved
RunDevelopment and others added 3 commits May 7, 2024 09:03
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
joeyballentine and others added 2 commits May 10, 2024 09:03
Co-authored-by: Michael Schmidt <mitchi5000.ms@googlemail.com>
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@joeyballentine joeyballentine merged commit 6fa0abe into main May 10, 2024
4 checks passed
@joeyballentine joeyballentine deleted the themes branch May 10, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants