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

feature: maybe set DiagnosticOk highlights #255

Closed
fredrikfoss opened this issue Mar 21, 2024 · 13 comments · Fixed by #290
Closed

feature: maybe set DiagnosticOk highlights #255

fredrikfoss opened this issue Mar 21, 2024 · 13 comments · Fixed by #290

Comments

@fredrikfoss
Copy link

Describe the solution you'd like

Hey, this is minor but I by chance saw that the DiagnosticOk highlight isn't set by the colorscheme. Right now it uses guifg=NvimLightGreen. I don't mind the color, but the DiagnosticVirtualTextOk highlight won't get a blended background like the other 4 ones :)

Additional context

$ nvim -v
NVIM v0.10.0-dev+2663-gc1c6c1ee1
Build type: RelWithDebInfo
LuaJIT 2.1.1707061634
Run "nvim -V1 -v" for more info
@mvllow
Copy link
Member

mvllow commented Mar 22, 2024

Do you have a screenshot/example of when this highlight is used? Happy to add support here but not sure how to test

@sjclayton
Copy link
Contributor

I also don't mind the color of NvimLightGreen and think it fits in with the rose-pine palette quite nicely.

which-key.nvim is now using DiagnosticOk as it's base linking for green to display it's icons if you don't use icon colors inherited from mini.icons

@fredrikfoss
Copy link
Author

You can test by checking out the highlights section of the diagnostic help page:

:h diagnostic-highlights
:h hl-DiagnosticOk
:h hl-DiagnosticVirtualTextOk

@sjclayton
Copy link
Contributor

sjclayton commented Jul 20, 2024

@fredrikfoss

We will have to wait for @mvllow to weigh in more on this... like I said I am fine with using the green color NvimLightGreen and could go a head and submit a PR to add highlights for both DiagnosticOk and DiagnosticVirtualTextOk.

But like I said, I'd like to see if there is anything @mvllow has to say about this first.

@mvllow
Copy link
Member

mvllow commented Jul 20, 2024

Ideally we would use a colour from our existing palette but we do have this super secret leaf green #6d8f89 in some of our brand material. We've used it atop the main and dawn base backgrounds so hopefully that could look good between all variants. Haven't tested personally but if someone wants to share a screenshot of it in use that'd be much appreciated :)

@sjclayton
Copy link
Contributor

@mvllow

I am going to do some testing, and play around with that color a bit and see what it looks like applying it to various things in my config... and I will post the results here.

@sjclayton
Copy link
Contributor

@mvllow

From the little bit of early testing I've done so far.... the leaf green color looks OK.... it's still a bit dark for my liking, in order to have it look good across all variants I think the saturation should be increased about 10%... so it would end up as #629a90, without increasing the saturation moon is the only variant it looks too dark on...

@mvllow
Copy link
Member

mvllow commented Jul 20, 2024

I found #769B94 deep in the archives, which does seem close to your new colour and looks a bit better on moon than the original I sent over.

@sjclayton
Copy link
Contributor

sjclayton commented Jul 20, 2024

I just tried it out, it looks ok... I still think #629a90 looks better and it is a direct relation of the original leaf green you sent just boosted a tiny bit, where as the new one is more lightened than boosted.

If you think #629a90 is too saturated, we could go in between and only do 5% more than your original suggestion which is #68948d which does also look good.

Here is a screenshot of all three side-by-side, let me know what you think.

2024-07-20_17-13

EDIT: I just tried the new one - #769b94 with dawn, and it doesn't look as good as the first one you sent, or my proposal.

@mvllow
Copy link
Member

mvllow commented Jul 20, 2024

I think I misspoke initially. From our branding material, #6d8f89 is the leaf colour for dawn and #769b94 is the leaf colour for main/moon. We can tweak them of course but they should be different per light/dark variants. I'm pretty happy with the dawn colour, but for main/moon we could lighten it to perhaps #95b1ac?

Edit for clarity: my current proposal is #95b1ac for main/moon and #6d8f89 for dawn :)

@sjclayton
Copy link
Contributor

@mvllow

I'm good with your current proposal. I was also wondering if this would mean this color set is added to the palette as a named color or no? If so I would just call it 'leaf' 😃

@mvllow
Copy link
Member

mvllow commented Jul 21, 2024

If you want to add leaf to the palette file that’s fine with me. I don’t see it going much further than our Neovim theme for now but it doesn’t hurt :)

@sjclayton
Copy link
Contributor

sjclayton commented Jul 21, 2024

@mvllow

I will be submitting additional PR's that rely on #290, to update some existing plugin support to include the new color.

Don't get me wrong, I love the palette, however I think there are situations where it can be hard to adequately achieve proper styling with only 6 colors... Neovim is one of those situations. I wasn't thinking you were going to necessarily add leaf to the main palette for everything.

Thanks for your input with this!! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants