search

roobert / tailwindcss-colorizer-cmp.nvim

:rainbow: A Neovim plugin to add vscode-style TailwindCSS completion to nvim-cmp
323 stars 3 forks source link

feat: support extra prefixes on black and white colors #8

Closed imNel closed 1 year ago

imNel commented 1 year ago

Noticed that extra prefixes weren't supported on black and white so I made a quick fix :)

This takes the logic used for the rest of the colors and applies it to b/w too. I also moved the boolean to a function and put it into a utils file, probably doesn't need to be in another file but it does seperate stuff nicely imo

NOTE: Formatting is still weird because my neovim wouldn't format it the same as your code :/

roobert commented 1 year ago

Thanks for this contribution!

I'd not bother splitting the function out into a separate file but instead include it in the main file since the size of this plugin is so small.

For formatting, I use stylua. Please could you also run your code through it and ensure the PR has the same style as the rest of the code, including spacing between blocks, etc.

After looking at the code a bit more, I think it might make more sense to rename the function to something like: color_position() that returns an int rather than having a function which returns a boolean which then gets translated into an index. It may not even be worth breaking this out into a function since it could be inlined and assigned to a variable which could then be referenced in the two locations it's used.

I used to functionalize everything, but more recently I tend towards inline style, if you're interested in why, you might like: http://number-none.com/blow/john_carmack_on_inlined_code.html

Also note that the index isn't specific to black and white, so shouldn't be prefixed with bw_.

Thanks!

imNel commented 1 year ago

Thanks for the feedback, I've applied it all to the pr in the new commit!

Firstly, can't believe I haven't heard of stylua before... Very handy! (I don't code in lua much)

Secondly, I definitely overestimated what I needed to do to achieve this functionality lol. I've moved the code up above and used the variable twice, a lot better than the function returning a bool or int :)

Also going to quickly comment on a couple things just for clarification 😄

roobert commented 1 year ago

Thanks again for this contribution!