search

ziontee113 / color-picker.nvim

A powerful Neovim plugin that lets users choose & modify RGB/HSL/HEX colors.
MIT License
283 stars 8 forks source link

<Plug>Close #11

Closed sassanh closed 2 years ago

sassanh commented 2 years ago

Thanks for this great plugin! I'd like to suggest adding <Plug>CloseColorPicker map key. Also please note that <Plug>ClearActionGroup is dangerous naming as potentially another vim plugin may use the same name, as described here it is better to prefix map names with script[/plugin] name. I thought now that color-picker.nvim is in early stages changing key map names wouldn't cause too much trouble for users.

ziontee113 commented 2 years ago

Hi, thank you for opening the issue.

  1. I assume so the user can remap to some other keys close the picker?
  2. Thank you for bringing that up. This is the first time I implement <Plug>. I assume the new Plugs should look something like <Plug>ColorPickerClearActionGroup? If I were to implement this, I'm planning to still keep the old ones in place (so it doesn't break existing users config) and edit the README to the new Plugs.
sassanh commented 2 years ago
  • I assume so the user can remap to some other keys close the picker?

Yes but then they have to add an autocmd for color-picker filetype and define the mapping there if they want that mapping to work only for color picker windows. Also same reasoning applies for <Plug>ApplyColor, the user can use autocmd approach to map something to <cr> but we provide it for the convenience which is the way to go for modern vim plugins as far as I know.

2. Thank you for bringing that up. This is the first time I implement <Plug>. I assume the new Plugs should look something like <Plug>ColorPickerClearActionGroup? If I were to implement this, I'm planning to still keep the old ones in place (so it doesn't break existing users config) and edit the README to the new Plugs.

With a semicolon in the end: <Plug>ColorPickerClearActionGroup; al least that's what the docs suggest, I don't have much experience with the concept too. If you keep the old ones it doesn't serve the purpose, to avoid possible conflict with other plugins we need to remove the old ones, it would be a breaking change unfortunately.

ziontee113 commented 2 years ago

Yeah.. I just realized adding the proper ones & still keeping the old ones is pointless. Changing them to proper Plug is easy, the hard part is handling public relations.. Which I have no experience at :cry:

I was looking at the option to create a new branch, and points the packer to choose that branch, and that branch will go on as the "main" branch if the user wants the new features. And that's another can of worms by itself.

Or... OK I've found it. So the user uses the setup function to use those Plug mappings. I just transform those to the proper Plug mapping if they're still using the old ones, so nothing is "broken" at the surface. And update the README so new user use the proper Plug mapping.

sassanh commented 2 years ago

Yeah, fortunately user doesn't do the mapping themself and just provides it in a table for the setup function and you can transform it. Good idea!

ziontee113 commented 2 years ago

I've decided to go with the format <Plug>ColorPickerClearActionGroup. I think it's clear enough that it shouldn't be any conflict with other plugins. I took at look at other plugins that uses <Plug> and almost none of them use ; at the end. Also having a ; at the end just feels weird for some reason.

ziontee113 commented 2 years ago

Hi @sassanh , the changes are now live. Please update the plugin. Thank you again for opening the issue. Please let me know if there's anything wrong with the changes :+1:

sassanh commented 2 years ago

Thanks! I appreciate your quick response.