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

feat: themes #3

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

nakajimayoshi
Copy link
Contributor

@nakajimayoshi nakajimayoshi commented Jul 7, 2023

Closes #7

Adds support for global themes beyond light/dark modes, keeping the original theme as the default.

Philosophy:

Why not just use tailwind dark:

As of now tailwind doesn't support dynamic themes beyond light/dark mode. State-based styling adds much more flexibility and customization options.

### Why even use tailwind then with all this css?
The CSS is condensed as much as possible, with the theme-specific dynamic styles being separated from tailwind concerns into style attributes.

### Why are you passing the ThemeConfig into the redux state machine and not the UITheme?
ThemeConfig is a simpler, serializable interface that can be passed to a UITheme on state changes. The existence of UITheme serves to make the items handled by the state alterable in the future (e.g. font style, size, etc.)

Theme values overview:

--primary : Primary color for highlighting text, elements, etc.
--secondary: Secondary highlight for visual contrast
--text: main text color
--padding: color for element padding/darker elements such as cards & buttons
--workspace-padding: lighter color than padding to account for extra layers in workspace
--background: main background color

Remaining issues:

  • Reduce code duplication between tailwind config file and uiThemes.ts
  • Add selected indicator for theme menu
  • Theme selection should persist after program exit
  • Adjust styles of homepage
  • Adjust styles of workspace
  • Adjust styles of Modals
  • Should Settings have its own path or should it just be a modal?
  • Add more themes

Theme:'Default(dark)'

image
image

Theme: 'FlyByWire(dark)'

image
image

Theme: Atom-one-dark

image
image

Theme: Monokai

image
image

@nakajimayoshi

This comment was marked as resolved.

@MikeRomaa
Copy link
Member

It seems to work for me? Unless you have some uncommitted code.

@nakajimayoshi

This comment was marked as outdated.

src/components/Card.tsx Outdated Show resolved Hide resolved
@MikeRomaa MikeRomaa mentioned this pull request Jul 10, 2023
@nakajimayoshi nakajimayoshi marked this pull request as ready for review July 11, 2023 07:23
src/index.tsx Outdated Show resolved Hide resolved
src/pages/Home.tsx Outdated Show resolved Hide resolved
src/pages/Settings.tsx Outdated Show resolved Hide resolved
@IAmVisco
Copy link
Contributor

Getting an error and can't start up ACE without any theme in the local storage. || 'amethyst-dark' is not a valid JSON to parse, so it errors out.

Copy link
Member

@MikeRomaa MikeRomaa left a comment

Choose a reason for hiding this comment

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

Really liking this so far, but I'm honestly not a fan of the manually defined class names in CSS. Can we look into using one of these solutions? The package tw-colors looks really promising so maybe we can mess around with that?

@nakajimayoshi
Copy link
Contributor Author

nakajimayoshi commented Jul 11, 2023

@MikeRomaa For the first option, it will expand the color pallette quite a bit so it will take time to refactor since not only will all the class names need to change, and their invocations, but I will have to experiment with ranges of each theme color up between ranges (e.g. 100-300)

This option will be limited in the fact that each theme MUST have the same amount of color variants for each field, or the error handling will spiral out of control (e.g. you can’t have FlyByWire go up to bg-bg-200 and amethyst go up to bg-bg-300)

tw-colors seems to be more or less the same implementation here since everything is prefixed with theme, and you’d have to add additional wording to distinguish the background, text, padding, etc.

I can try shortening the classnames a bit and changing secondary to accent.

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.

Customization options and themes
3 participants