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

feat: add variant="light|dark" option to override &background #496

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

williamhCode
Copy link
Contributor

PR for the discussion here: #491 (reply in thread)

I have a few questions about the code. Right now, refresh_icons() is called directly in the nvim-web-devicons.lua file. However, now that I'm adding an option which the refresh_icons() function depends on, should I move the call into the setup() function? Right now im just calling refresh_icons() again if the variant option is not "auto", which is a bit inefficient.

@williamhCode williamhCode force-pushed the add-variant-option branch 2 times, most recently from 3315fbe to 74bbc57 Compare September 15, 2024 14:56
@alex-courtis
Copy link
Member

Right now im just calling refresh_icons() again if the variant option is not "auto", which is a bit inefficient.

That's unavoidable; we can't do anything until we know their choice.

Setup actually expects the icons to be populated i.e. refresh_icons has been called: https://github.com/williamhCode/nvim-web-devicons/blob/83238ce4809b4a34282ca8148e26e2d3fbf09ff7/lua/nvim-web-devicons.lua#L397

Suggestion: move the refresh_icons call at 595 to setup.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Works beautifully, :NvimWebDeviconsHiTest shows correct colours and highlighting in all tests.

README.md Outdated Show resolved Hide resolved
lua/nvim-web-devicons.lua Outdated Show resolved Hide resolved
@williamhCode
Copy link
Contributor Author

Additionally, i moved the refresh to right after setting variant global opt

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Simple and bulletproof, tested OK again.

Many thanks for your contribution!

@alex-courtis alex-courtis changed the title feat: add variant option feat: add variant="light|dark" option to override &background Sep 20, 2024
@alex-courtis alex-courtis merged commit 31bd21a into nvim-tree:master Sep 20, 2024
5 checks passed
@williamhCode
Copy link
Contributor Author

No problem!

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