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

Theme Support #146

Merged
merged 7 commits into from
Jul 16, 2022
Merged

Theme Support #146

merged 7 commits into from
Jul 16, 2022

Conversation

itspngu
Copy link
Contributor

@itspngu itspngu commented Jul 10, 2022

Summary

This PR adds some initial/basic support for user-configurable themes. I didn't want to turn too many things inside out, which is why
there's a "shim" function that translates the user's color choices into the currently-used format; the idea being that the default theme ships a combined light + dark mode, but once a user writes their own theme, the complexity of that is unnecessary, as they're writing it for their own terminal, which is either light or dark.

On top of that, it's possible to set a useShellTheme bool in the config file, which skips custom themes altogether and instead uses the user's shell theme. I'm not sure if it looks good for everyone, perhaps that needs some testing/adjustment and maybe also the same light+dark logic of the default theme.

In summary, the config file generated on the first run now includes this (these are the colors for a dark terminal, for a light one it'd dump a different set of colors - I "transcribed" the default color scheme):

theme:
  colors:
    text:
      main: '#E2E1ED'
      secondary: '#666CA6'
      bright: '#E2E1ED'
      subtle: '#242347'
      faint: '#3E4057'
      warning: '#F23D5C'
      success: '#3DF294'
    background:
      selected: '#39386B'
    border:
      main: '#383B5B'
      secondary: '#39386B'
      faint: '#2B2B40'

I've deliberately put everything under a theme.colors sub-key, because looking at the open issues, there's a few things that'd probably fit well into a theme.layout (#107) or theme.options (#111) tree.

How did you test this change?

Barely, and I'm new to Go. Opening this as a draft to facilitate discussion about the implementation. When/if everything is agreed on code wise, I'd update the docs in a separate commit. Also looking at base16 to generate a set of themes, license wise gh-dash is compatible with it, but again, don't want to do too much at once.

Images/Videos

image

<making the robots happy>
Fixes #102
</making the robots happy>

If there's anything unclear, ask away, I have trouble articulating myself sometimes! :)

@dlvhdr
Copy link
Owner

dlvhdr commented Jul 10, 2022

Hey! Amazing stuff, thanks so much for contributing this! I'll go over it in the next couple of days 💖

ui/styles/theme.go Outdated Show resolved Hide resolved
ui/styles/theme.go Show resolved Hide resolved
config/parser.go Outdated Show resolved Hide resolved
@dlvhdr
Copy link
Owner

dlvhdr commented Jul 13, 2022

I made a whole mess with the colors :D
I think some are unused, and some are just badly named.
I left comments I think should make things more clear.

theme:
  colors:
    text:
      main: '#E2E1ED' # should probably be renamed to "primary"
      secondary: '#666CA6'
      bright: '#E2E1ED' # I actually searched the code and I don't think this is used
      subtle: '#242347' # This one is badly named. It's used for the pills which have a light background color and so need a dark text. Maybe it should be named "inverted"?
      faint: '#3E4057'
      warning: '#F23D5C'
      success: '#3DF294'
    background:
      selected: '#39386B'
    border:
      main: '#383B5B' # "primary"
      secondary: '#39386B'
      faint: '#2B2B40'

@itspngu

This comment was marked as resolved.

@itspngu itspngu marked this pull request as ready for review July 16, 2022 13:45
@itspngu itspngu requested a review from dlvhdr July 16, 2022 13:45
Copy link
Owner

@dlvhdr dlvhdr left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes!

I forgot about the whole markdown renderer colors. Is that something you'd want to tackle in this PR maybe?
It could be a bit much for this one. If you don't have time etc, I can take it in a later PR.
I just noticed it doesn't look very good on light themes.

https://github.com/dlvhdr/gh-dash/blob/d25d8101d5717a4e10ae83c2d33ae8d1fb7a3a08/ui/markdown/theme.go

ui/styles/theme.go Outdated Show resolved Hide resolved
ui/styles/theme.go Show resolved Hide resolved
Comment on lines 41 to 50
SelectedBackground: lipgloss.AdaptiveColor{Light: "006", Dark: "006"},
Border: lipgloss.AdaptiveColor{Light: "000", Dark: "015"},
FaintBorder: lipgloss.AdaptiveColor{Light: "007", Dark: "008"},
SecondaryBorder: lipgloss.AdaptiveColor{Light: "008", Dark: "007"},
FaintText: lipgloss.AdaptiveColor{Light: "243", Dark: "249"},
MainText: lipgloss.AdaptiveColor{Light: "000", Dark: "015"},
SecondaryText: lipgloss.AdaptiveColor{Light: "237", Dark: "255"},
SubleMainText: lipgloss.AdaptiveColor{Light: "015", Dark: "015"},
SuccessText: lipgloss.AdaptiveColor{Light: "002", Dark: "002"},
WarningText: lipgloss.AdaptiveColor{Light: "001", Dark: "001"},
Copy link
Owner

Choose a reason for hiding this comment

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

I played around with the colors on a dark theme and light theme terminals and I think I like these colors better:

			SelectedBackground: lipgloss.AdaptiveColor{Light: "006", Dark: "008"},
			Border:             lipgloss.AdaptiveColor{Light: "013", Dark: "008"},
			FaintBorder:        lipgloss.AdaptiveColor{Light: "254", Dark: "000"},
			SecondaryBorder:    lipgloss.AdaptiveColor{Light: "008", Dark: "007"},
			FaintText:          lipgloss.AdaptiveColor{Light: "007", Dark: "249"},
			MainText:           lipgloss.AdaptiveColor{Light: "000", Dark: "015"},
			SecondaryText:      lipgloss.AdaptiveColor{Light: "244", Dark: "251"},
			SubleMainText:      lipgloss.AdaptiveColor{Light: "015", Dark: "236"},
			SuccessText:        lipgloss.AdaptiveColor{Light: "002", Dark: "002"},
			WarningText:        lipgloss.AdaptiveColor{Light: "001", Dark: "001"},

Let me know what you think!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works well on my default shell and Kitty, but on Konqueror it's somewhat borked. I'm not really passionate about this because I can make my own theme if I don't like the default now ;)

image
image

Copy link
Owner

Choose a reason for hiding this comment

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

Cool then! I'll change the ansi colors to mine as I prefer the borders not to be high contrast.
But I too will just use my custom theme :)

@dlvhdr
Copy link
Owner

dlvhdr commented Jul 16, 2022

Overall looks great!

@itspngu itspngu changed the title Theme Support - Initial Version/Draft Theme Support Jul 16, 2022
Copy link
Owner

@dlvhdr dlvhdr left a comment

Choose a reason for hiding this comment

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

You can push whenever you're ready.
Thanks again!

@itspngu
Copy link
Contributor Author

itspngu commented Jul 16, 2022

You can push whenever you're ready.

I'm not sure what you mean..? :D

@dlvhdr
Copy link
Owner

dlvhdr commented Jul 16, 2022

Ah push = merge

@itspngu
Copy link
Contributor Author

itspngu commented Jul 16, 2022

I can't, it's set up so only you can :P

image

@dlvhdr
Copy link
Owner

dlvhdr commented Jul 16, 2022

Whoops ok then :)

@dlvhdr dlvhdr merged commit cf6a461 into dlvhdr:main Jul 16, 2022
@itspngu itspngu deleted the add-theme-support branch July 16, 2022 18:28
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.

Support color themes
2 participants