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

theme: ✨ add default and update the others optional #591

Merged
merged 6 commits into from
Jan 16, 2022

Conversation

zwpaper
Copy link
Member

@zwpaper zwpaper commented Nov 28, 2021

fix #549

difference:

  1. use a default value as fallback color instead of falling back to the default theme, one may copy the default theme value from README
  2. no options for the terminal default foreground color, still suffering how to represent this

TODO

  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry

@zwpaper zwpaper mentioned this pull request Nov 28, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2021

Codecov Report

Merging #591 (3224e73) into master (fba600d) will increase coverage by 0.23%.
The diff coverage is 95.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #591      +/-   ##
==========================================
+ Coverage   85.89%   86.12%   +0.23%     
==========================================
  Files          37       37              
  Lines        3849     3899      +50     
==========================================
+ Hits         3306     3358      +52     
+ Misses        543      541       -2     
Impacted Files Coverage Δ
src/meta/mod.rs 39.51% <0.00%> (ø)
src/color.rs 51.49% <50.00%> (+0.30%) ⬆️
src/color/theme.rs 71.75% <100.00%> (+18.00%) ⬆️
src/meta/size.rs 95.40% <100.00%> (ø)
src/app.rs 72.58% <0.00%> (ø)
src/meta/name.rs 92.88% <0.00%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fba600d...3224e73. Read the comment docs.

@meain
Copy link
Member

meain commented Nov 29, 2021

Hi @zwpaper, I have not yet fully gone through the codebase but I was thinking that a better way to implement the idea of default colors is to have it use the colors from the default theme when no color is mentioned. For example, if the user has not defined the color for the size block, it will lookup the color in the default theme for size.

The idea is that they can mostly use the default colors, but just update only the ones that they don't like. The way you have done it will mean the user will have to define every single color if they need to change the colors.


On a related note, I was thinking that we could improve how we pick up the theme file. We could make it so that if a file called theme.yaml is present in the .config/lsd directory, we use that for the theme. This might reduce confusion on the theme file naming as seen in some issues. But yeah, this can pushed for later.

@zwpaper
Copy link
Member Author

zwpaper commented Nov 29, 2021

Hi @meain, thanks for the input, as I mentioned in the description, I also wondering how should we handle the default color.

This PR could be a small Proof of Concept and discussion.

a fallback to the default theme is something like patch the default theme, I was thinking how about letting people create some brand new themes, and sharing or switching themes.

so that a fixed default value seems better than falling back to the default theme, also, people could copy and modify the default theme from README if this is what they want.

this also answers your second question, why I did not choose to use theme.yaml as the theme file name and create a theme dir for the themes.

@meain
Copy link
Member

meain commented Nov 30, 2021

I think it is better to still fallback to the default theme instead of using a default value.

  1. It makes it so that if someone want to change just the date column, they don't have to copy paste the full theme
  2. There are no values that has to be there (default), everything becomes optional
  3. default will usually be the base color which will anyways not suit if we were to introduce something new like a new optional block.

Besides, people who plan to create full new things can always just make sure they cover everything by starting off with the default theme and updating all the values.

@zwpaper zwpaper changed the title WIP: theme: ✨ add default and update the others optional theme: ✨ add default and update the others optional Jan 7, 2022
@zwpaper zwpaper marked this pull request as ready for review January 7, 2022 14:56
@zwpaper zwpaper requested review from meain and Peltoche as code owners January 7, 2022 14:57
@zwpaper
Copy link
Member Author

zwpaper commented Jan 7, 2022

@meain now this feature should works as expected

Copy link
Member

@meain meain left a comment

Choose a reason for hiding this comment

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

This looks great. The only thing that I would add here an option for the user to choose the default terminal foreground (reset in crossterm, something like a none option in theme file) so that they will not have to specify back/while etc and have it work in both light and dark setups, but that came come in later.

I think we could probably go for the next release as well now.

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.

Handling default/none values in theme config
3 participants