noahgorstein / jqp

A TUI playground to experiment with jq
MIT License
2.19k stars 40 forks source link

feat: custom themes #40

Closed mluna-again closed 1 year ago

mluna-again commented 1 year ago

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 commented 1 year ago

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

noahgorstein commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mluna-again commented 1 year ago

Done, let me know what you think!

noahgorstein commented 1 year ago

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