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

rfc: new Search highlight #305

Closed
mvllow opened this issue Jul 25, 2024 · 24 comments
Closed

rfc: new Search highlight #305

mvllow opened this issue Jul 25, 2024 · 24 comments

Comments

@mvllow
Copy link
Member

mvllow commented Jul 25, 2024

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

@mvllow
Copy link
Member Author

mvllow commented Jul 25, 2024

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
Copy link
Contributor

sjclayton commented Jul 25, 2024

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
Copy link
Member Author

mvllow commented Jul 25, 2024

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
Copy link
Member Author

mvllow commented Jul 25, 2024

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
Copy link
Contributor

That looks really good actually... 😲

@mvllow
Copy link
Member Author

mvllow commented Jul 25, 2024

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
Copy link
Contributor

sjclayton commented Jul 25, 2024

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
Copy link
Member Author

mvllow commented Jul 25, 2024

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
Copy link
Member Author

mvllow commented Jul 25, 2024

Screenshot 2024-07-25 alle 17 26 33 Screenshot 2024-07-25 alle 17 26 52

@sjclayton
Copy link
Contributor

sjclayton commented Jul 25, 2024

HOLY SHIT!! 😮

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

@sjclayton
Copy link
Contributor

sjclayton commented Jul 25, 2024

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
Copy link
Member Author

mvllow commented Jul 26, 2024

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
Copy link
Contributor

sjclayton commented Jul 26, 2024

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
Copy link
Member Author

mvllow commented Jul 26, 2024

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
Copy link
Contributor

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

@sjclayton
Copy link
Contributor

sjclayton commented Jul 26, 2024

What are the settings you used in the last screenshot?

@mvllow
Copy link
Member Author

mvllow commented Jul 26, 2024

Ugh

Screenshot 2024-07-25 alle 21 03 54

@mvllow
Copy link
Member Author

mvllow commented Jul 26, 2024

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

@sjclayton
Copy link
Contributor

Using which settings did that happen?

@mvllow
Copy link
Member Author

mvllow commented Jul 26, 2024

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
Copy link
Contributor

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

@sjclayton
Copy link
Contributor

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

@mvllow
Copy link
Member Author

mvllow commented Aug 25, 2024

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

@mvllow mvllow closed this as completed Aug 25, 2024
@mvllow mvllow reopened this Aug 25, 2024
@mvllow
Copy link
Member Author

mvllow commented Aug 25, 2024

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.

@mvllow mvllow closed this as completed in d396005 Oct 8, 2024
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

No branches or pull requests

2 participants