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

fix curly brace matching #162

Closed
wants to merge 1 commit into from
Closed

fix curly brace matching #162

wants to merge 1 commit into from

Conversation

redbmk
Copy link

@redbmk redbmk commented Mar 19, 2018

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
Copy link
Author

redbmk commented Mar 19, 2018

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
Copy link
Owner

mxw commented Mar 19, 2018

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.

@mxw mxw closed this Mar 19, 2018
@redbmk
Copy link
Author

redbmk commented Mar 19, 2018

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.

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

Successfully merging this pull request may close these issues.

Matching Curly Brace Highlight Wrong
2 participants