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

Kanagawa: fix bufferline, theme wrap-indicators, cursor, menu, and syntax changes #6085

Merged
merged 8 commits into from
Feb 26, 2023

Conversation

luetage
Copy link
Contributor

@luetage luetage commented Feb 23, 2023

This is a followup to #5571

  • The active bufferline syntax has an error (inactive ⇒ active)
  • I removed virtual in the previous pull request, because it wasn’t used. Turns out this was a mistake, the newly introduced wrap‐indicator depends on it.

These two fixes are needed, everything that follows is miscellaneous improvements and fixes.

  • I found out completion menus in the original Kanagawa use waveBlue1/2 as background and implemented it

  • Non‐text characters (newline/nbsp) become invisible when highlighted, changed them to use sumiInk4

  • The primary and secondary cursor highlight are hard to distinguish, they have better contrast now for multiple selections. I also changed cursor.match to waveRed for the same reason.

  • Added cursorcolumn color; this was already themed through primary cursorline, but it can’t hurt to write it down

  • I played around with markup modifiers, added and changed a few highlights. Comparison below:

    markupbefore markupafter

  • I ordered the syntax according to documentation, made tag and label different for CSS, changed attributes to waveRed from peachRed because it was a bit aggressive in HTML, where many attributes are in use. Added a few additional highlights while trying to keep it consistent. I removed module highlight, because it isn’t mentioned in the documentation and I removed superfluous entries (e.g. for punctuation everything is one color, therefore only one entry is needed).

@zetashift and @leonqadirie: Sorry to annoy you again so soon ^^ but please have a look, if you have time. Some of this is probably biased and as mentioned earlier, only the first two points really need attention. I’m willing to throw all the other commits out should you dislike them.

@the-mikedavis the-mikedavis added A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 23, 2023
@leonqadirie
Copy link
Contributor

leonqadirie commented Feb 24, 2023

LGTM. Thank you!

--
I welcome all PRs and improvements to my favorite (Rust) theme, no matter the frequency (not a challenge, though!).
I'm personally not a fan of the completion menu - but if the original uses waveBlue1/2 I'd adhere to it here and override in my private setup.

@luetage
Copy link
Contributor Author

luetage commented Feb 25, 2023

@leonqadirie Purely a matter of taste; the menu changes aren’t needed and most of the other changes aren’t essential either. If you like it, we include it, if not, I won’t be disappointed, because I can likewise run these changes just for myself. Thanks for the feedback in any case 💐

@leonqadirie
Copy link
Contributor

leonqadirie commented Feb 25, 2023

@luetage No, I'm with you, let's follow the original here.
Saying this as a simple one-time PR contributor of @zetashift's 'community effort' ;)

@zetashift
Copy link
Contributor

Following the original is always the better route!
These look like great additions so thank you :D.

p.s. no need to say sorry when tagging, I very much appreciate these kinds of PRs and the work you have done.

@the-mikedavis the-mikedavis merged commit cac4a36 into helix-editor:master Feb 26, 2023
estin pushed a commit to estin/helix that referenced this pull request Mar 4, 2023
wes-adams pushed a commit to wes-adams/helix that referenced this pull request Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants