mxw / vim-jsx

React JSX syntax highlighting and indenting for vim.
1.59k stars 95 forks source link

fix curly brace matching #162

Closed redbmk closed 6 years ago

redbmk commented 6 years ago

fixes #146

Without this change there were three issues:

  1. Incorrect curly braces were highlighting
  2. In neovim, pressing % would move to the highlighted, incorrect brace
  3. The colors between curly braces as a jsx attribute were highlighted the same color as an attribute, rather than as plain javascript, and the first curly brace was the same color as an xml string. Now everything between the braces is the same color as an xml string
redbmk commented 6 years ago

This still isn't quite right...I found code like this will cause issues (1) and (2) to occur:

render() {
  return (
    <Item thing={<OtherItem field={something} />} />;
  );
}

Also, are the colors supposed to be the same as an XML String, or the same as Javascript code? I would expect it to render like Javascript, but that's not the case before or after this patch (before the patch it was rendering like an attribute, and after it's rendering like a string).

mxw commented 6 years ago

Right—this change is broken with any nontrivial amount of nesting of blocks and brace-enclosed strings. I'm not familiar with neovim or its paren-matching algorithms, so I can't speak to (2).

(3) is a legitimate issue with this approach, I believe (though I'm not fully parsing your sentence describing it). I'm not sure what the right approach is to fix it, however. If you come up with something, feel free to update this PR or submit a new one.

redbmk commented 6 years ago

Regarding the comment on the colors (3), I just noticed I had written an incomplete sentence, so I edited that. Also, I figure some pictures might help illustrate what I'm trying to say.

Before the patch, an xml string ("string") is one color. For the javascript attributes, { is one color (the same color as a string), and jsVariable} is another color (the same color as the attribute name itself - in this case, data-javascript-variable): image

After the patch, the full {jsVariable} portion is colored the same as "string". This seems more correct than before the patch: image

I feel like the best result however would be for { and } to be the same color as "string", and the inner portion (jsVariable) to be javascript colored (offwhite in this colorscheme). Here's an edited screenshot of what that would look like: image

Or another option would be for {jsVariable} to be colored as if it were plain javascript (offwhite in this colorscheme), like in this mockup: image

FWIW I'm using gruvbox as a colorscheme. I tried a few other themes to see if I could get something a little more clear than two colors of green, but it looks like quite a few render strings and attributes the same color.