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: Solarized themes #68

Merged
merged 2 commits into from
Dec 10, 2019
Merged

feat: Solarized themes #68

merged 2 commits into from
Dec 10, 2019

Conversation

hakamadare
Copy link
Contributor

adding Solarized (light/dark) themes from
https://github.com/paulcpederson/solarized-sublime

@dandavison
Copy link
Owner

Awesome, thanks! I believe we want to add Solarized (light) to the list of light themes, so that delta uses the background colors for a light terminal background when we do delta --theme='Solarized (light)'. Could you consider adding the following:

diff --git a/src/style.rs b/src/style.rs
index 4868d12..bbb05a2 100644
--- a/src/style.rs
+++ b/src/style.rs
@@ -1,10 +1,11 @@
 use syntect::highlighting::{Color, FontStyle, Style, StyleModifier};
 
-pub const LIGHT_THEMES: [&str; 4] = [
+pub const LIGHT_THEMES: [&str; 5] = [
     "GitHub",
     "Monokai Extended Light",
     "OneHalfLight",
     "ansi-light",
+    "Solarized (light)",
 ];
 
 pub const DEFAULT_LIGHT_THEME: &str = "GitHub";

(or you can add me as a collaborator on your fork/PR; I'm not quite sure what the workflow is supposed to be in this situation.)

@dandavison
Copy link
Owner

I don't think I'm seeing any difference between the foreground colors used by light vs. dark. WDYT, should these look different?

image

My commands there are

  • git show 07f9421 | delta --theme='Solarized (dark)'
  • git show 07f9421 | delta --theme='Solarized (light)'

@dandavison
Copy link
Owner

dandavison commented Dec 6, 2019

Do you know whether it would make sense to add these themes to bat also, or have a suspicion why it hasn't been done already? I think there are some considerations regarding color themes for terminal emulators that I haven't understood yet (see e.g. sharkdp/bat#490).

@hakamadare
Copy link
Contributor Author

I don't think I'm seeing any difference between the foreground colors used by light vs. dark. WDYT, should these look different?

the color schemes should not look dramatically different; one of the features of Solarized is that its overall look and feel does not change when switching from light to dark mode (https://ethanschoonover.com/solarized/#features)

solarized-dark-vs-light

see the difference between the two different versions of the same content? in the dark mode (upper image) the base text is Solarized color "base 0", in the light mode the base text is Solarized color "base 00"

@hakamadare
Copy link
Contributor Author

Do you know whether it would make sense to add these themes to bat also, or have a suspicion why it hasn't been done already? I think there are some considerations regarding color themes for terminal emulators that I haven't understood yet (see e.g. sharkdp/bat#490).

i'm not opposed to opening a similar PR against bat; the message i took from the bat documentation was that the author wants people to compile their own theme/style bundles and load them at runtime, whereas with delta the theme and style bundles are compiled in.

@dandavison
Copy link
Owner

Thanks!

the message i took from the bat documentation was that the author wants people to compile their own theme/style bundles and load them at runtime

There are definitely accepted PRs adding themes to bat. cc @sharkdp would adding solarized themes to bat be a helpful contribution?

whereas with delta the theme and style bundles are compiled in.

Right, currently. But I intend to (a) support all themes that bat supports by default (i.e. update delta periodically to match bat) and (b) support reading custom themes and syntaxes from the same directory that bat uses (~/.cache/bat), see #19 and #58 (comment).

@dandavison dandavison merged commit 7951807 into dandavison:master Dec 10, 2019
@hakamadare
Copy link
Contributor Author

done! sharkdp/bat#768

@hakamadare hakamadare deleted the solarized-themes branch December 11, 2019 04:41
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.

2 participants