sainnhe / gruvbox-material

Gruvbox with Material Palette
MIT License
1.96k stars 166 forks source link

Wrong colors for tag and tag.attribute for nvim-treesitter-tsx? #177

Closed tomasjm closed 1 year ago

tomasjm commented 1 year ago

Hello, I don't know if a problem of the theme, the parser or my configuration. I think the color is wrong from what can be seen in the image for "tag" and "tag.attribute" when using Tree-sitter "tsx" parser (nvim-treesitter-tsx) using nvim 0.9.0. :TSInstall tsx

screenshot

image image

I was trying to add colors by linking but I failed. I think that apparently other highlight groups are overriding the colors? Due to my limitation of treesitter knowledge I can't do more.

I want to learn to fix it with some help, if this is a problem of the theme, so I can add support for tsx if is necessary

antoineco commented 1 year ago

Nice catch.

I know nothing about tsx, but it looks like those tags are being categorized as Constructor, and Tag with a lesser priority.

Could you please try adding this autocmd to your Neovim config, and see whether the look and feel improves?

-- Apply custom highlights on colorscheme change.
-- Must be declared before executing ':colorscheme'.
local grpid = vim.api.nvim_create_augroup('custom_highlights_gruvboxmaterial', {})
vim.api.nvim_create_autocmd('ColorScheme', {
  group = grpid,
  pattern = 'gruvbox-material',
  callback = function()
    local function hl_lnk(src, trgt)
      vim.api.nvim_set_hl(0, src, { link = trgt })
    end

    hl_lnk('@constructor.tsx', '@type')
    hl_lnk('@tag.attribute.tsx', '@property')
  end
})

image

antoineco commented 1 year ago

One drawback of implementing this change in the colorscheme is that actual constructors are now highlighted like @type.

image

Luckily there aren't that many tokens identified as @constructor in JS/TS (I think). However, I'm a bit reluctant to make this the default, because we would be changing the semantics globally just to get around the current behaviour of Neovim's tsx queries. It would be great if these queries could highlight React types in markup tags as @tag.constructor—or anything else under @tag really—so that colorschemes can apply meaningful highlights in markup blocks.

Maybe something worth suggesting upstream. The parser is already identifying everything correctly, so it would be only a matter of tuning the queries to separate React tags (constructors) and attributes from their JavaScript/TypeScript counterpart.

image image

tomasjm commented 1 year ago

Hey, thank you, now I understand a little more the problem and learn how to set colors in a good way.

I was testing with some other colorschemes like catppuccin and it seems that they are coloring tsx files differently than ts files. Actually is strange to put class definitions in .tsx files if not React with Class Components imo, so I think that the constructor sharing the color is not that bad, also thinking that tsx files will have more jsx as are used principally for React projects and .ts will probably have more classes.

Others themes are also separating tag and constructor color attributes for html tags and react components.

Tried with some colors to copy, as an example, catppuccin behaviour only for .tsx files with function and class component:

highlight! link @tag.tsx Red
highlight! link @constructor.tsx Orange 
highlight! link @tag.attribute.tsx Blue 

image image

Probably exists a better match of colors to follow the theme guidelines, but I like a lot the readability of the example

EDIT: Another example using only html tags (red) and another using jsx components (orange):

ex1 ex2
antoineco commented 1 year ago

OK thanks for your feedback 👍

It's unfortunate that two tokens of the same color ended up next to each other in React. I see that catppuccin also added the exact same overrides for tsx files, so they probably ran into the same issue.

I trust your jugement regarding the rare usage of constructors in tsx files and will implement the change in the colorscheme whenever I find the time to do it. As colorscheme authors we have to remain faithful to the themes' semantics[^1] and provide consistent defaults.

Samples:

tsx

image

html (no change)

image

[^1]: The linked highlights are for Everforest, but Everforest and Gruvbox Material share similar semantics. I'm still working on porting this document to Gruvbox Material.

tomasjm commented 1 year ago

Thank you! It looks good now. I will give more feedback if I find issues while working with tsx