Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hightlight groups for nvim-treesitter #230

Merged
merged 4 commits into from
Mar 5, 2021
Merged

Add hightlight groups for nvim-treesitter #230

merged 4 commits into from
Mar 5, 2021

Conversation

DerekStride
Copy link
Contributor

What

The neovim project nvim-treesitter/nvim-treesitter ands some additional highlight groups for better syntax highlighting. I've adapted the work done by @nitishvelu in ChristianChiarulli/nvcode-color-schemes.vim#14 to support the treesitter hightlighting and would like to get it merged upstream in the main dracula/vim plugin.

Rust

Left: Highlighting with nvim-treesitter

Right: Existing highlighting

Screen Shot 2021-03-04 at 13 03 32

Ruby

Left: Highlighting with nvim-treesitter

Right: Existing highlighting

Screen Shot 2021-03-04 at 13 05 21

The majority of this configuration was taken from
ChristianChiarulli/nvcode-color-schemes.vim#14
by [@nitishvelu](https://github.com/nitishvelu). I made a few minor
tweaks based on a ruby project to make sure most of the original tokens
were colored similarly.
Copy link
Member

@dsifford dsifford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @DerekStride.

Looks good for the most part. Can you make sure the changes are aligned with the spec?

I added a few comments of things that stuck out at me, but I didn't comb through each group you added.

Once that's done, this should be good to merge.

after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
@DerekStride
Copy link
Contributor Author

Can you make sure the changes are aligned with the spec?

Thanks for the quick review @dsifford, I dug through the spec and updated the attributes I could reproduce in the latest commit 32fd093.

Copy link
Member

@dsifford dsifford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline suggestions.

It's important to use the builtin highlight groups wherever feasible because if someone were to switch color schemes, all of the linked dracula groups would still remain linked if the new color scheme does not re-link them. This leads to annoying bugs in some cases, particularly for those who tend to change color schemes on the fly a lot.

after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
after/plugin/dracula.vim Outdated Show resolved Hide resolved
Comment on lines +97 to +98
hi! link TSTitle DraculaYellow
hi! link TSLiteral DraculaYellow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what these are specifically, but headings are purple and bold and inline code delimited with backticks is green.

colors/dracula.vim Outdated Show resolved Hide resolved
colors/dracula.vim Outdated Show resolved Hide resolved
DerekStride and others added 2 commits March 5, 2021 10:24
Co-authored-by: Derek Sifford <dereksifford@gmail.com>
Co-authored-by: Derek Sifford <dereksifford@gmail.com>
@dsifford
Copy link
Member

dsifford commented Mar 5, 2021

LGTM. We can refine from there if needed. Thanks @DerekStride!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants