Skip to content

Conversation

@daschw
Copy link
Contributor

@daschw daschw commented Feb 3, 2023

Hi, thanks for this awesome piece of software!

I just recently came across this project and played around with the theme config a little. At some point I thought it would be nice to have darker borders. However, I could not find this option anywhere in the component styles. So I thought I'd try to implement this myself. After I had finished, I realized that this would require every user to update their theme.toml files because without the new border option spotify_player would not be able to parse them and error. Furthermore I saw that you recently refactored the theming config and deleted fg_selected and bg_selected (or how they were called) and I saw the TODO comment to find a solution to not having to set each component style when adding this section in theme.toml.

Hence I thought, before I add an option to change the border I'll try to figure out a way to solve this TODO. Afterwards, adding new component styles would not be an issue, because it would not affect older configs, that did not set this attribute. So that is what I ended up doing. With this PR

  • it is possible to only set some of the component styles in theme.toml
  • it is possible to not set the palette in theme.toml (and just modify component styles)
  • it is possible to set the border style with the new component_style attribute border
  • it is possible to set the style of the current selection with the new component_style selection

I am quite sure that none of these changes affect any existing theme configs.

With this I could now have a theme.toml like this

[[themes]]
name = "mine"
[themes.component_style]
border = { fg = "BrightBlack" }
selection = { fg = "Yellow", modifiers = ["Bold"] }

that would look like this with my terminal colors:

Screenshot_20230203_203849

@aome510 aome510 self-requested a review February 3, 2023 22:07
@aome510
Copy link
Owner

aome510 commented Feb 3, 2023

Hi @daschw, thanks for your interests in this project and the effort you put in making this PR.

I'll take a look and review this PR the next few days.

Copy link
Owner

@aome510 aome510 left a comment

Choose a reason for hiding this comment

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

This PR is easier to review than I initially thought. Generally, it LGTM. Only a few small changes are needed.

@aome510
Copy link
Owner

aome510 commented Feb 3, 2023

[[themes]]
name = "mine"
[themes.component_style]
border = { fg = "BrightBlack" }
selection = { fg = "Yellow", modifiers = ["Bold"] }

Tried your theme and it also works for me!

@daschw
Copy link
Contributor Author

daschw commented Feb 3, 2023

Hi, thanks for the review! I implemented all your suggestion. I was not sure, where exactly to put the default 'fg' and 'bg' value description instead of the "border comment". Let me know, if this is not what you had in mind.

Copy link
Owner

@aome510 aome510 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 the changes. LGTM!

@aome510
Copy link
Owner

aome510 commented Feb 3, 2023

I also update the example theme and theme_parse script by myself.

@aome510 aome510 merged commit e47e6df into aome510:master Feb 3, 2023
@daschw daschw deleted the theme branch February 3, 2023 23:15
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