search

nordtheme / vim

An arctic, north-bluish clean and elegant Vim theme.
https://www.nordtheme.com/ports/vim
MIT License
2.52k stars 274 forks source link

Add support for vim-clap #178

Closed meck closed 4 years ago

meck commented 4 years ago

This adds highlights for vim-clap

clap
arcticicestudio commented 4 years ago

Hi @meck :wave:, thanks for your contribution :+1: I'll review it in the near future and get back to you.

Arelav commented 4 years ago

Can this be merged, please?

sdemura commented 4 years ago

I would love to see this merged!

arcticicestudio commented 4 years ago

Sorry for this delay, life, other projects and tons of Nord issues/PRs keeps me busy 😐 I haven't checked and tested the actual code, but I'd like to throw in some thoughts regarding the design. Nord's main accent is nord8 that can be combined with the other colors of the Frost palette. While Aurora colors like nord12 or nord13 might be used more often in other themes, Nord tries to keep they purpose as signal colors to style e.g. warnings and errors. Therefore I'd like to propose to change the style for Vim Clap to use

I use fzf as my main fuzzy finder for almost everything so I also use it together with fzf.vim. While there's no official Nord port project for it (yet) I created a Nord theme for my personal use that is embedded into my ZSH .dotfiles This is what is looks like when using the design I've described above:

Thanks again for the active feedback on this PR, really like the way the community works on such things 😄 Let me know what you think if we'd use this design for Vim Clap too.

meck commented 4 years ago

I've cleaned this up a bit and conformed it to @arcticicestudio's suggestions, however I noticed Clap now has a auto-loaded theme system as used in liuchengxu/vim-clap#420. So it should probably be moved there (The same as airline and sightline)? Also all the providers have their own hl-groups, I've added a couple but the rest should probably be added as well.

Then there is the question if there should be a theme file in here and in the clap repo as well? I guess that is something for @liuchengxu and @arcticicestudio to agree on.

liuchengxu commented 4 years ago

Then there is the question if there should be a theme file in here and in the clap repo as well?

The advantage of putting it in the colorscheme repo is when people use this colorscheme, they'll have the clap support aumatically and don't have to add the line let g:clap_theme = 'nord' explicitly. So I would second the former way if the clap theme has a original vim colorscheme.

arcticicestudio commented 4 years ago

This is always a question that depends on the targeted application. Having builtin theme support in plugins is comfortable for users because in most cases they only need to specify the theme name in their configuration, but this comes with the price of maybe outdated themes and possible incompatibles. Some examples where incompatibles have been the root cause are #187 and #193 where the users installed a plugin called flazz/vim-colorschemes that includes themes for vim-airline/vim-airline theme, among other theme for Nord.

Contrary to my own statement in the pull request that merged Nord's airline theme into the origin repository, this indeed adds maintenance overhead. For example #128 updated the airline theme included in this repository to be more compatible with the edkolev/tmuxline.vim plugin, but this means the theme in the airline repository is now outdated. Normally you would now have to assume that the maintainers of the airline plugin are monitoring the repositories of the themes that are also included in their repository, but to be truth no one can expect that, least of all if everyone works on such projects in their free time.

On the other side, adding support for other plugins in a color scheme is a better way to separate the actual dependencies between both and prevent conflicts: One-direction dependency from Nord → vim-clap. This way only Nord as color scheme affects the actual highlighting that is managed by Vim while vim-clap doesn't need to care about the color scheme at all. Adding highlighting rules in both now adds a two-direction dependency because now vim-clap actually cares about the highlighting, but might be affected when the user also sets Nord as active color scheme that includes settings for vim-clap specific highlighting groups. Updating one of them might result in different styles when used with or without Nord as active color scheme.

I don't know how theme loading has been implemented in vim-clap, but it should ensure to not override highlighting settings of the currently active Vim color scheme to also prevent incompatibility problems. But since the PR in the vim-clap repository has already been merged, I guess there's no more reason to decide which solution might fit :smile:

Anyway, I hope to find some free time again to work on Nord so I can finally test this PR and merge it.

arcticicestudio commented 4 years ago
Release Notes Assets