-
Notifications
You must be signed in to change notification settings - Fork 331
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
Support icon color #1674
Support icon color #1674
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the patch, overall looks good. The only downside to this approach is that you can't specify icon colors using environment variables, but it's probably not worth doing anyway.
In addition, I suggest you should:
- Change the unit tests for
readPairs
to testreadArrays
instead. - Update the documentation for icons.
- Add some examples (commented out) to the
etc/icons.example
config file.
misc.go
Outdated
} | ||
|
||
pairs = append(pairs, pair) | ||
pairs = append(pairs, arr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might want to rename this variable:
pairs = append(pairs, arr) | |
arrs = append(arrs, arr) |
@joelim-work Updated, check again pls. 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks once again for the changes.
etc/icons.example
Outdated
@@ -359,3 +375,212 @@ Vagrantfile | |||
|
|||
# other formats | |||
*.pdf | |||
|
|||
# icons with color |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OPTIONAL: This is just an idea, maybe it might be worth moving this into a separate file (e.g. icons_colored.example
). But either way is OK for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a good idea too. Already moved.
Thank you very much @wodesuck for the patch and @joelim-work for the review. There is some conflict in this patch after #1644 is merged, and I wasn't sure how to resolve them. Can you please update the patch one more time? Sorry for the inconvenience as we have been holding many feature patches for a while due to the new release. |
@gokcehan Conflict resolved. |
Implement #928 #1520. Now you can add the third column in
~/.config/lf/icons
to set icon colors, fallback to filename color in~/.config/lf/colors
if icon color is not specified.Example:
Also implement #992. You can remove icons by writting only one column in
~/.config/lf/icons
, for example: