search

f4z3r / gruvbox-material.nvim

Material Gruvbox colorscheme for Neovim written in Lua
MIT License
32 stars 4 forks source link

Fixed variable text colors. Changed treesitter capture names links #12

Closed dot-1q closed 3 months ago

dot-1q commented 3 months ago

This PR fixes the variable text color, which was previously blue. This was not consistent with gruvbox-material. I also made the treesitter capture names more consistent, in light of how gruvbox-material does it. By default, neovim adjusted the spacing on the comments. If this is not wanted, I can create another PR that does not contain those changes.

Note: I'm sorry for the mess that happened while creating the PR, I pressed enter when typing the PR tittle and it automatically submitted it without me providing additional information

f4z3r commented 3 months ago

Hi @dot-1q , no worries and thanks for opening the PR!

I am a bit unsure about this. Mostly I am a bit unsure whether we should strive to be 100% consistent with sainnhe/gruvbox-material. I took over this repo from WittyJudge, and I am mostly trying to keep the colorscheme consistent with how it originally was. My reasoning here is that people that are using it probably don't want it to change too much during an update.

An option to allow consistency with sainnhe's plugin would be to provide a configuration flag that enables a "compatibility mode". However, that would be quite difficult to ensure that it truly follows sainnhe's colorscheme considering it still gets updates from time to time.

Regarding the links: thanks! I wanted to get to that at some point but never got around to it. I want to clean-up the links everywhere (not only treesitter stuff) so that it is consistently linking back to something that makes sense. Considering that the new @ captures are much more precise than the old TS* highlight groups, would it not make more sense to perform the link the other way around? What do you think?

Finally, it seems that your linking changes quite a bit more than just variable text colors. See below, the original is on the left, and your branch on the right: image

dot-1q commented 3 months ago

Hi @f4z3r, You are correct that my changes resulted in a bit more than the variable text colors, although my goal was to better approximate the current state of the palette to the original work done by sainnhe, as the README stated this project as a port. As you've pointed out, this is not your goal currently.

Left: This colorscheme | Right: gruvbox-material from sainhe.

ss

The main reason I changed the ["@variable"] = { link = "TSVariable" } ,which then has TSVariable = { link = "Fg" } was because this color behaviour did not feel correct, as there's too much blue color on the screen.

As for the links, now that I'm looking at them again, and seeing how other colorschemes handle them, such as tokyonight, and imo the best course of action is to follow its structure, which is to link the neovim highlight-groups directly to the color palette, as you've done already, and then link treesitter to the highlight groups when their linking name makes sense, else, assign them directly from the palette.

With all this said, I think its best to close this PR, and I'll try to follow up with one which only takes care of the links this weekend. Thank you for your work!