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

Add themes #34

Merged
merged 7 commits into from Nov 7, 2022
Merged

Add themes #34

merged 7 commits into from Nov 7, 2022

Conversation

bashbunni
Copy link
Contributor

Changes made:

  • Created a Theme struct for new themes and used Catppuccin themes as a starting point.

Supported themes:

  • Catppuccin "mocha", "macchiatto", and "latte" (light theme)
  • Default (anything other than those values

One thing I noticed and haven't been able to debug is that the first time I run gocovsh it gives a nil pointer reference, but the second time I run it, there's no problem. Attached a GIF for your reference which includes a demo of both the error and the working style for latte theme.

gocovsh-themes

#4

@orlangure
Copy link
Owner

Hi @bashbunni, thank you for working on this, and for adding Catppuccin to the project (I wanted to try it for a while).

Regarding the nil pointer, I'm unable to reproduce it, and from the stack trace it seems that the problem is somewhere deeper in the bubbles list or some other related library. Have you noticed anything like that with previous versions?

Regarding the code and the feature, I like it! I would love to take it further in the future, and allow to provide configuration files in addition to environment variables. This change wouldn't be breaking because you already used GOCOVSH_ prefix. We'll allow using a theme: key in yaml config or similar env vars in the future. I'm also thinking about configurable colors in the yaml files in addition to pre-defined themes.

Unfortunately, there is a regression in this PR: colors in the list view are broken (maybe related to the nil pointer?). As seen from test output, the selected lines are no longer green. Will you be able to address this, or should I look for a fix myself?

And again, thank you for the contribution 😼

@orlangure
Copy link
Owner

@bashbunni, I suggested a fix to the regression in https://github.com/bashbunni/gocovsh/pull/1. If you have a better solution, feel free to close my PR 😼

@bashbunni
Copy link
Contributor Author

Thanks so much for your help! I'll take another stab at resolving the error - I had a feeling it might be on the Bubble Tea side. I'll work on fixing that! Also happy to add the option to use a config file as well.

Copy link
Owner

@orlangure orlangure left a comment

Choose a reason for hiding this comment

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

This is so cool! Thank you.

There were some linter warnings, but I really wanted to merge this, so I just pushed my fixes directly, hope you don't mind.

@orlangure orlangure merged commit 16a7273 into orlangure:master Nov 7, 2022
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.

None yet

2 participants