-
Notifications
You must be signed in to change notification settings - Fork 185
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
feat: add variant="light|dark" option to override &background #496
Conversation
3315fbe
to
74bbc57
Compare
74bbc57
to
6d87ada
Compare
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. |
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.
Works beautifully, :NvimWebDeviconsHiTest
shows correct colours and highlighting in all tests.
Additionally, i moved the refresh to right after setting variant global opt |
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.
Simple and bulletproof, tested OK again.
Many thanks for your contribution!
No problem! |
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 thenvim-web-devicons.lua
file. However, now that I'm adding an option which therefresh_icons()
function depends on, should I move the call into thesetup()
function? Right now im just callingrefresh_icons()
again if the variant option is not "auto", which is a bit inefficient.