rose-pine / neovim

Soho vibes for Neovim
MIT License
2.36k stars 151 forks source link

rfc: new `Search` highlight #305

Closed mvllow closed 1 month ago

mvllow commented 4 months ago

304 changed Search to be "rose". This is a discussion for making it neutral and not stand out as much as "text".

mvllow commented 4 months ago
require("rose-pine").setup({
    highlight_groups = {
        Search = { bg = "muted", blend = 20, inherit = false },
    },
})
Screenshot 2024-07-25 alle 16 56 59

I like using shades of "muted". I actually wanted to replace our "highlight_" colours with different shades of "muted". Either way, we need to take into consideration cursorlines, visual mode, etc. to ensure this looks okay in many circumstances.

sjclayton commented 4 months ago

That looks fine for dawn but on moon and main its not very visible. I went with rose because I tried various combos of other colors with various blends and nothing seemed to work as well on both dawn and the dark variants.

mvllow commented 4 months ago

It is tough to balance–this is why we ended up changing the bg to "text". I agree it can be sharp, but I guess I'm not usually leaving search visible for long

mvllow commented 4 months ago
require("rose-pine").setup({
    highlight_groups = {
        CurSearch = { fg = "base", bg = "rose", inherit = false },
        Search = { bg = "rose", blend = 20, inherit = false },
    },
})
Screenshot 2024-07-25 alle 17 09 10 Screenshot 2024-07-25 alle 17 11 36

Sort of interesting using a solid for the current search and blended for others

sjclayton commented 4 months ago

That looks really good actually... 😲

mvllow commented 4 months ago

Only concern would be if we want to change the foreground for Search. I think for CurSearch it's fine—when on the current search you're pretty aware of your context. Leaving the foreground alone for other searches could be useful to maintain context but also leads to some harder to read text:

Screenshot 2024-07-25 alle 17 13 53

And with a different foreground:

Screenshot 2024-07-25 alle 17 16 06
sjclayton commented 4 months ago

I don't know that leaving the Search foreground alone is that necessary for maintaining context, I think it looks fine with it changed.

But changed to what, would be the question..?

mvllow commented 4 months ago
require("rose-pine").setup({
    highlight_groups = {
        CurSearch = { fg = "base", bg = "rose", inherit = false },
        Search = { fg = "rose", bg = "rose", blend = 20, inherit = false },
    },
})

I love pine on dawn, but unfortunately not that readable on dark variants. Rose is probably the most versatile between variants.

BUT WHAT IF LEAF IS THE ANSWER TO IT ALL

mvllow commented 4 months ago
Screenshot 2024-07-25 alle 17 26 33 Screenshot 2024-07-25 alle 17 26 52
sjclayton commented 4 months ago

HOLY SHIT!! 😮

Literally not my first thought but.... WOW, that looks incredible.

sjclayton commented 4 months ago

I just tried

  CurSearch = { fg = 'base', bg = 'leaf', inherit = false },
  Search = { fg = 'leaf', bg = 'leaf', blend = 20, inherit = false },

on moon which is my main variant, and the only thing is maybe we could increase the blend by 10?? it seems just a touch too dark at 20.

Maybe my monitor is causing an issue with the appearance, because it looks somewhat different than the screenshot you posted. Although that shouldn't be the case....

mvllow commented 4 months ago
Screenshot 2024-07-25 alle 19 18 47 Screenshot 2024-07-25 alle 19 18 31

This is a blend of 30, the text is slightly hard to read. A blend of 20 does give us WCAG AA contrast for moon.

As far as dawn, that's not a passing contrast. Changing leaf to #516767 does get us there but it changes leaf quite a bit.

Might need to noodle this—I would say the blend of 20 would be fine to merge now to me, and I'm happy to revisit leaf as a whole to make it more accessible. Open to further discussion though

sjclayton commented 4 months ago

Alrighty - 20 blend is good, since it is better for contrast. Was 20 the blend you used in the original screenshot using leaf anyways?

Just for note the contrast issue I was referencing earlier was between the bg color of Search and the base background color of the editor, not the foreground text of Search itself. If that wasn't already clear.. 😄

mvllow commented 4 months ago

I think I narrowed it down to these two options for accessibility. The "text" variant is the best contrast and most neutral (of course). The "leaf" variant is a little more fun but honestly I'm leaning towards the neutral for the theme and the user is more than welcome to update it in their config. We could add all of these as recipes in the wiki.

Text

Screenshot 2024-07-25 alle 20 58 26 Screenshot 2024-07-25 alle 20 58 12

Leaf

Screenshot 2024-07-25 alle 20 57 53 Screenshot 2024-07-25 alle 20 57 28
sjclayton commented 4 months ago

I'd be ok with the neutral variant as in this form it is way less obtrusive than the original implementation. 👍🏻

sjclayton commented 4 months ago

What are the settings you used in the last screenshot?

mvllow commented 4 months ago

Ugh

Screenshot 2024-07-25 alle 21 03 54
mvllow commented 4 months ago

Just realised my cursor (for Ghostty at least) is invisible on the current search

sjclayton commented 4 months ago

Using which settings did that happen?

mvllow commented 4 months ago
CurSearch = { fg = "base", bg = "text", inherit = false },
Search = { fg = "text", bg = "text", blend = 20, inherit = false },
CurSearch = { fg = "base", bg = "leaf", inherit = false },
Search = { fg = "text", bg = "leaf", blend = 20, inherit = false },
sjclayton commented 4 months ago

Using kitty here and I don't have that issue, my cursor automatically gets inverted.

sjclayton commented 4 months ago

I am going to spend some time with the neutral implementation and see how it looks and feels during use.

mvllow commented 3 months ago

Changed the default to be neutral and added the leafy example to the wiki recipes :)

mvllow commented 3 months ago

Well shoot, I completely forgot this messes with the cursor colour on some terminals. Using text should probably be avoided... although cursor-invert-fg-bg = true fixes this in Ghostty.

I am using the leaf version personally and quite like it but may need to noodle some more.