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: custom themes #40

Merged
merged 1 commit into from
Sep 17, 2023

Conversation

mluna-again
Copy link
Contributor

Hi, I wanted to use a theme that wasn't available and at first I thought about making a PR to add the theme, but then I realized that I would also have to make a PR in the chroma repo to add the theme there as well, so I figured it would be nice to be able to use a custom theme directly.
Let me know what you think!

@noahgorstein
Copy link
Owner

Thanks for your PR. Should be able to look at this soon!

@noahgorstein
Copy link
Owner

Hey @mluna-again - thanks again for your contribution! (and sorry for taking a bit to review 😅 ) I do like the idea for sure. Question tho - if I'm understanding correctly, this will allow a user to configure like the non-syntax highlighting theme. Essentially override the lipgloss styles used in the app - separate from the syntax highlighting theme. It looks your PR uses the default chroma styles (vim and paradaiso light) for syntax highlighting.

https://github.com/mluna-again/jqp/blob/76888750bb3a4908053f1e56a7c9a38812141cc8/tui/theme/theme.go#L464-L468

If this is the case, I'm curious if we should just add configuration options such that theme takes the same values it always has but introduces the following config/overrides you've added for?

  primary: "#c4b28a"
  secondary: "#8992a7"
  error: "#c4746e"
  inactive: "#a6a69c"
  success: "#87a987"

What do you think? I guess I think that "custom" theme might mean to a user that they can control every aspect, in which case I'd expect to be able to change the syntax highlighting theme and the lipgloss styles.

@mluna-again
Copy link
Contributor Author

mluna-again commented Jul 25, 2023

Hello @noahgorstein, don't worry about it! Thanks for your response.
Yes, that's right, you could already override the syntax highlighting with chromaStyleOverrides so this PR enables the user to change the remaining colors.

But yeah I also think the "custom" value could be confusing, do you have a specific format in mind for the overrides? I was thinking something like this could work:

  theme:
    name: "monokai"
    overrides:
      primary: "#c4b28a"
      secondary: "#8992a7"
      error: "#c4746e"
      inactive: "#a6a69c"
      success: "#87a987"
    chromaStyleOverrides:
      ...

What do you think? maybe styleOverrides instead of just overrides to be more consistent with chromaStyleOverrides?

@noahgorstein
Copy link
Owner

Hm both are fine options IMO but perhaps styleOverrides would give us flexibility down the road to have other overrides in the app like keybindings. What's your preference?

@mluna-again
Copy link
Contributor Author

I like it, I'll update the PR. Thanks!

@mluna-again
Copy link
Contributor Author

Done, let me know what you think!

@noahgorstein
Copy link
Owner

Done, let me know what you think!

looks great, thanks @mluna-again . Sorry have been quite busy but looking to ship a release v soon

@noahgorstein noahgorstein merged commit 304eacc into noahgorstein:main Sep 17, 2023
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