Skip to content

Conversation

acidghost
Copy link
Contributor

@acidghost acidghost commented Feb 12, 2025

Closes #48.

The only thing I don't necessarily like is being forced to alias hours='hours --theme ...'.
Do you think we can introduce an environment variable to set a default value for the theme (e.g. HOURS_THEME)?

@acidghost acidghost marked this pull request as draft February 12, 2025 09:30
@acidghost
Copy link
Contributor Author

Draft since there's a conflict with -t flag of active command.

I also want to add some tests.

@dhth
Copy link
Owner

dhth commented Feb 12, 2025

Thanks for the PR :)
Having a bit of a busy week; will try to take a look at it by the end of the week.

Yes, falling back to HOURS_THEME in the absence of the --theme flag sounds good. If the env var is also not set, we fall back to hours'ss built in theme.

@acidghost acidghost marked this pull request as ready for review February 12, 2025 15:07
@dhth
Copy link
Owner

dhth commented Feb 21, 2025

Thanks again for the PR. And apologies for the delay in reviewing it.

Overall, the approach looks solid. I've created a PR to your fork with some tweaks that I think make sense. I've listed the details in that PR's description.

Take a look, and lemme know if anything looks odd or is unclear. Would be great if you could give the new subcommands a spin and generally test out a bunch of workflows.

@acidghost
Copy link
Contributor Author

I enabled "Allow edits by maintainers", feel free to push on this PR directly.

@dhth dhth merged commit a33ed69 into dhth:main Feb 22, 2025
9 checks passed
@dhth
Copy link
Owner

dhth commented Feb 22, 2025

Thanks for your contribution! :)

@acidghost acidghost deleted the feat/theming branch February 22, 2025 10:31
@acidghost
Copy link
Contributor Author

Was a pleasure working on this repo!

I've been using hours daily at work for the past 3/4 weeks, it's amazing!

I have a couple use-cases that are not fully covered, expect more contributions 😉

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.

Allow customization of some colors
2 participants