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

Improve theme usage + add default ones #1274

Merged
merged 8 commits into from
Apr 1, 2022
Merged

Improve theme usage + add default ones #1274

merged 8 commits into from
Apr 1, 2022

Conversation

kunalmohan
Copy link
Member

  • We no longer use gray color in the plugins and is no longer required in the theme.

Samples-

Solarized Dark
solarized-dark

One Half Dark
onehalf-dark

Molokai Dark
molokai-dark

Gruvbox Light
gruvbox-light

Gruvbox Dark
gruvbox-dark

Dracula
dracula

@kunalmohan kunalmohan temporarily deployed to cachix March 26, 2022 11:20 Inactive
@kunalmohan kunalmohan temporarily deployed to cachix March 26, 2022 11:20 Inactive
@kunalmohan kunalmohan temporarily deployed to cachix March 26, 2022 16:37 Inactive
@kunalmohan kunalmohan temporarily deployed to cachix March 26, 2022 16:37 Inactive
@imsnif
Copy link
Member

imsnif commented Mar 31, 2022

This is fantastic! One minor issue: this changed the default colors for me.

Before:
img-2022-03-31-091133

After:
img-2022-03-31-091110

Was this intentional?

@kunalmohan
Copy link
Member Author

Was this intentional?

Yeah I switched the background color of the status bar from gray to black (since gray is removed no longer specified in the theme). Maybe we can set a lighter shade of black in the default_palette, if that's better?

@imsnif
Copy link
Member

imsnif commented Mar 31, 2022

Hum - would that affect other stuff? Do we use this black color somewhere else in our UI?

@kunalmohan
Copy link
Member Author

Ah..right. Black is used for text on the status bar. Using a lighter color may reduce the visibility of the text as well. But we can still change it to 236, it is lighter and still properly visible.

image

@imsnif
Copy link
Member

imsnif commented Apr 1, 2022

Hmm... still not great. I just feel it would be a pity if we have to change our UI because we changed our API. Do you feel there's no other way?

@kunalmohan
Copy link
Member Author

I don't think there's a way to maintain our current default UI other than by "hacking" the plugin specifically for when the default color palette is used. This may also confuse the user since the background and text color would be different when using the default theme and would be the same when using a user-defined theme. Users would expect the same behaviour when using themes.
Maybe we should consider keeping gray in the user-defined theme? The reason I removed it is because it may not be specified in many default themes available online and it will save the user the effort to find the right shade between white and black defined in the theme.
IMO the new UI, with black=236, looks good too. I think it's just a matter of being used to the current UI that makes the change look not so good initially.

@imsnif
Copy link
Member

imsnif commented Apr 1, 2022

Okay, that's fair. I understand the tradeoffs now and when you put it this way, I totally agree. Let's go with 236. Otherwise this LGTM. Great work on this! Merge whenever you're ready :)

@kunalmohan kunalmohan temporarily deployed to cachix April 1, 2022 14:28 Inactive
@kunalmohan kunalmohan temporarily deployed to cachix April 1, 2022 14:28 Inactive
@kunalmohan kunalmohan merged commit 9f71648 into main Apr 1, 2022
@kunalmohan kunalmohan deleted the fix-colors branch April 1, 2022 21:49
@bluss
Copy link

bluss commented Apr 27, 2022

Thanks for adding themes. My Zellij usage can now start - with a theme that works for a light terminal. 🙂

This pull request was closed.
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.

3 participants