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

feat(which-key): v3 support #1323

Closed
wants to merge 1 commit into from
Closed

Conversation

channinghsu
Copy link

This pull request updates the which-key mappings to the new specification as outlined in the documentation for version 3. The previous mappings were using the old specification, which caused warnings during the health check. The updated mappings now use the add method and are structured according to the latest guidelines.

Before:

5701720888312_ pic
5711720888477_ pic

After:

image

Copy link
Collaborator

@CharlesChiuGit CharlesChiuGit left a comment

Choose a reason for hiding this comment

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

wow, so fast!
LGTM

圖片
some icons are missing, but i think it's acceptable.

@Jint-lzxy Jint-lzxy changed the title Update which-key Mappings to New Specification feat(which-key): v3 support Jul 14, 2024
Copy link
Collaborator

@Jint-lzxy Jint-lzxy left a comment

Choose a reason for hiding this comment

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

LGTM! And u r suuuper fast lol But I kinda wanna address a few issues with regards to the new version, so I've opened another version that does the exact same thing but with some settings explicitly disabled, specifically:

  • Per feature: Disable which-key when first time get into visual mode folke/which-key.nvim#656 (comment), I disabled which-key in all modes other than normal and insert mode bc imo having that popup appear every time I'm about to make a selection is remarkably annoying and a major distraction.
  • Disabled the built-in icon support. afaiu folke seems to have used some pattern-matching magic to detect which icon to use, but to be blunt it's buggy and doesn't cover all cases (my observations). Plus, imho it's really just another layer of distraction since we've already assigned an intuitive icon to each top-level definition.

@ayamir
Copy link
Owner

ayamir commented Jul 16, 2024

Personally I prefer keep consistency with the old appearance. What's more, this PR seems also missing on the icons like this:
image
So I merged #1324, thanks for your contribution again.

@ayamir ayamir closed this Jul 16, 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

Successfully merging this pull request may close these issues.

4 participants