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

Incorrect syntax highlighting for JSON #648

Closed
grant opened this issue Jan 14, 2019 · 2 comments
Closed

Incorrect syntax highlighting for JSON #648

grant opened this issue Jan 14, 2019 · 2 comments

Comments

@grant
Copy link

grant commented Jan 14, 2019

Describe the bug
I'd expect JSON with primer-markdown to be highlighted with correct JSON highlighting.

To Reproduce

  1. Copy JSON.
  2. Preview in GitHub md file:
{
  "verbose": true,
  "ignore": ["*.test.js", "fixtures/*"],
  "execMap": {
    "rb": "ruby",
    "pde": "processing --sketch={{pwd}} --run"
  }
}

Expected behavior
String keys and values should be colored differently from tokens such as {, :, ,.

Screenshots

Expected

(Highlighting in VS Code)

screen shot 2019-01-14 at 12 48 40

Actual

screen shot 2019-01-14 at 12 48 46

Desktop (please complete the following information):

  • OS: OSX 10.13.6
  • Browser: Chrome
  • Version: 71.0.3578.98 (Official Build) (64-bit)
@shawnbot
Copy link
Contributor

Hi @grant! First of all, I completely agree that our syntax highlighting of JSON leaves a lot to be desired. However, "fixing" it is actually a lot more complicated than it might seem. Read on for the exciting backstory!

One underlying issue is that we actually don't do any syntax highlighting in Primer CSS (this repo). Fenced code blocks are rendered on github.com by a (closed-source, at least for now) tool that generates class attribute tokens compatible with the TextMate/Sublime Text/Atom family of grammars, and the github-syntax-light repo hosts the syntax theme for that specific grammar. Because syntax grammars vary so much from one highlighting library (and even language) to the next, it's not feasible for Primer to provide CSS that works with all of them.

That's just one part of the problem, though! If you look at the HTML that our syntax highlighter generates, you'll see that there actually aren't any unique classname "hooks" for us to style the key and value tokens differently:

input

{
  "hello": "world"
}

output (with whitespace for clarity)

<div class="highlight highlight-source-json"><pre>{
  <span class="pl-s">
    <span class="pl-pds">"</span>
    hello
    <span class="pl-pds">"</span>
  </span>:
  <span class="pl-s">
    <span class="pl-pds">"</span>
    world
    <span class="pl-pds">"</span>
  </span>
}</pre></div>

So yeah, it's complicated. ☹️

I'll bring this up with my team, but I'm going to close this for now because it's technically not a Primer issue. Thank you so much for bringing it to our attention, and for (I hope) sparking an important conversation about the inflexibility of our syntax highlighting setup.

@grant
Copy link
Author

grant commented Jan 14, 2019

OK. Thanks for the writeup. You're definitely the expert in navigating how this code structure works. I wasn't sure where to report this issue, so thanks for replying.
Even if keys and values were the same color, that would still be an improvement.

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

No branches or pull requests

2 participants