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

Add support for LSPSignatureActiveParameter #286

Conversation

cmoscofian
Copy link
Contributor

@cmoscofian cmoscofian commented Jan 22, 2022

Currently when calling vim.lsp.buf.signature_help on a given
function/method nord does not highlight or differentiate the active parameter.

This PR adds an accent color (nord8) and an underline to the current
active parameter making it stand out.

Highlight group docs: https://github.com/neovim/neovim/blob/70db972e5fbcab39946ad8ac05472a693cf65b68/runtime/doc/lsp.txt#L456-L459

See it in action:
Before:
nord-before-zoom

After:
nord-after-zoom

Live:
nord-1

Currently when calling `vim.lsp.buf.signature_help` on a given
function/method nord does not highlight or differentiate in any way the
active parameter.

This commit adds an accent color (nord8) and an underline to the current
active parameter making it stand out.
@arcticicestudio
Copy link
Contributor

Hi @cmoscofian 👋, thanks for your contribution 👍
I'll review and test the changes within the next week(s).

Copy link
Member

@svengreb svengreb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style looks good to me, but please move the code line into the already existing Neovim LSP block which is guarded for “vanilla” Vim users.

@cmoscofian
Copy link
Contributor Author

cmoscofian commented Feb 18, 2022

The style looks good to me, but please move the code line into the already existing Neovim LSP block which is guarded for “vanilla” Vim users.

Yeap, no problem. Since you mentioned it I noticed that there are a few neovim specific definitions outside of that if statement, that were all introduced on version 0.5 (Diagnostics, NativeLSP and Treesitter).

Should I give a whirl at a minor refactor and just subject them all under the same specific condition checking for 0.5?

Copy link
Member

@svengreb svengreb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I remember why the guard exists at all: in #282 we added support for the new groups and the ones that are guarded for Neovim version(s) 0.5.z are the deprecated/renamed ones, but I wanted to keep them for backwards compatibility reasons.
We can move all Neovim specific groups that are currently not guarded into the already existing block that checks if Neovim is used at all, but this is out of scope for this PR.

You can ignore my review comment and I'll merge this PR. Thanks again for contribution 🚀

@svengreb svengreb merged commit a825678 into nordtheme:develop Feb 19, 2022
@svengreb svengreb added this to the version-next milestone Feb 19, 2022
@svengreb
Copy link
Member

Release Note Assets

Before

After

@cmoscofian cmoscofian deleted the enhancement/lsp-signature-help-active-param-support branch February 19, 2022 14:16
crispgm added a commit to crispgm/nord-vim that referenced this pull request Apr 2, 2022
arcticicestudio pushed a commit that referenced this pull request May 14, 2022
The `vim.lsp.buf.signature_help` function is used to highlight the
active parameter in the signature help [1].
Before this commit the active parameter was not styled differently to
any other parameter which made it hard to distinguish it. This has been
improved by adding support for the `LspSignatureActiveParameter` syntax
highlighting group where the active parameter now uses `nord8` are
foreground color and additionally a font underline with the same color.

[1]: https://github.com/neovim/neovim/blob/70db972e5fbcab39946ad8ac05472a693cf65b68/runtime/doc/lsp.txt#L456-L459


Co-authored-by: Sven Greb <development@svengreb.de>
Co-authored-by: Arctic Ice Studio <development@arcticicestudio.com>

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

Successfully merging this pull request may close these issues.

3 participants