Add diff highlighting line transformer

[janosh]

Closes #62.

This is a draft implementation of a line transformer that enables diff highlighting on code fences with the {diff} option.

• Lines starting with +  indicate additions and can be highlighted with a green transparent background.
• Lines starting with -  indicate deletions and can be highlighted with a red transparent background.

This seems like a very common use case to me so I took the liberty of including createDiffLineTransformer in getDefaultLineTransformers. Of course, feel free to undo that if you think this is more niche.

Also, due to my complete lack of JSDoc knowledege, I'd like to appeal to CONTRIBUTING.md which states:

If types are new to you, or you have trouble getting type checking to pass, that’s ok! I’ll help you in your PR if necessary.

Specifically, I'm having trouble with the type declaration on the transformer returned by createDiffLineTransformer.

Finally (not sure if the infrastructure for this is implemented yet), how do I access a code fence'sparsedOptions in a line transformer?

const transformer = ({ line, parsedOptions: { diff } }) => { ... }

ain't working.

[andrewbranch]

Cool! Looking forward to taking a look soon!

[janosh]

I've time to work on this some more tomorrow in case you already have some feedback.

[andrewbranch]

Hey @janosh, sorry for the delay. I started to respond last weekend, but as I was reviewing, I realized there’s a problem, and I’ve been trying to think of a solution. First of all, though, thanks so much for continuing to contribute to this plugin!

So, the problem is that leaving the + and - characters in the source will mess up the syntax analysis, because they’ll be interpreted as part of the source code (e.g. as binary or unary operators in JavaScript), which will incorrectly influence the classification of the tokens they precede.

The only solution is to strip off those characters in the line transformer, and add them back with CSS or a custom renderer. (This would also solve the lesser problem of the characters affecting apparent indentation relative to lines that don’t have a +/- prefix, as the +/- character could be rendered in a gutter instead of pushing the rest of the line out to the right.) But, it seems like it could be difficult to do that in a way that looks right automatically for every theme. The same goes for the green/red line background color. You’ve set it to transparent by default, requiring users to pick colors that work for their theme(s), which matches the current behavior of the line highlighting transformer—but, via #51, I really want to change that and try to do some smart color analysis so highlighting works automatically. It seems like a strategy like that would be more difficult for green and red highlights, but I do want to get away from using transparent as a default in the future, because it makes it seem like the default experience is broken. These are tough problems.

The direction I’m leaning is that if we can’t provide defaults that work well for most users out of the box, I’d rather the transformer be a separate package that users can install, add to the plugin options, and configure however they need to. (If you decide to move this code to your own package, I’ll still be happy to help get it working as I think it’s good for the ecosystem.)

What are your thoughts?

[janosh]

@andrewbranch To me it sounds like you've already devised adequate solutions to all the problems you mention.

The only solution is to strip off those characters in the line transformer, and add them back with CSS or a custom renderer.

Adding them into the gutter with CSS would be good, I think. That seems to be exactly what VS Code does as well.

A custom renderer might be more hassle than is necessary.

But, it seems like it could be difficult to do that in a way that looks right automatically for every theme.

I don't see why. Could you elaborate?

but I do want to get away from using transparent as a default in the future, because it makes it seem like the default experience is broken.

Yes, I was thinking the same thing while creating this PR. But I forgot about #51 and that you're already planning to change this.

I checked how red and green highlighting looks on different dark backgrounds while working on this. Looked great to me with every theme I checked. And in fact, if you open a diff view in VS Code and cycle through the pre-installed dark themes, you'll see that all except one (High Contrast) use similar shades of green/red transparent backgrounds for highlighting added/removed lines. And the plus/minus signs in the gutter of added/removed lines are exactly identical across all themes. So I think just setting --grvsc-line-diff-(add|del)-background-color to the values in test/integration/viewer/render.js would do the job. But since people already specify a VS Code theme in the extension's config, shouldn't there be a way of adopting whichever values the theme specifies for --grvsc-line-diff-(add|del)-background-color?

One piece of functionality in VS Code that's missing entirely from this PR is that lines that are only modified are highlighted with a more transparent shade of green/red than lines that are completely new/deleted. Those get a stronger green/red background (see screenshot above). But figuring out the "newness" of a line would require significant code analysis and likely add a huge amount of complexity to this PR so I'm thinking we should not do that.

[andrewbranch]

Sorry for the delay. A couple responses:

But, it seems like it could be difficult to do that in a way that looks right automatically for every theme.

I don't see why. Could you elaborate?

Sorry, I think when I typed that, my brain went off in one direction and my fingers just typed “theme” instead. I think what I meant to say was that it could be difficult to introduce an ad-hoc gutter that wouldn’t disrupt any custom styles the user has put on the code block. Like, it’s tempting just to absolutely position a + or - floating on top of the line to the far left without changing the overall structure of how lines are laid out, but whether you have room to do that, and how far from the border you place it, would depend on the horizontal padding, which is configurable via CSS variable. I can easily see that variable set very small on mobile viewports, which wouldn’t leave enough room for the +/- character. That strategy could also interfere with line numbers (#28), which I think is in higher demand for an included line transformer. In a world where there’s both a line number option and a diff option, they would clearly need to work together in order not to step on each other’s toes.

I checked how red and green highlighting looks on different dark backgrounds while working on this. Looked great to me with every theme I checked. And in fact, if you open a diff view in VS Code and cycle through the pre-installed dark themes, you'll see that all except one (High Contrast) use similar shades of green/red transparent backgrounds for highlighting added/removed lines.

Ok, this is good to hear. That eliminates the color concern for me. If we can figure out how to deal with the gutter without introducing a ton of complexity, I’ll feel good about adding this.

[janosh]

If we can figure out how to deal with the gutter without introducing a ton of complexity, I’ll feel good about adding this.

That's great! I'll think about this tomorrow.

[janosh]

How do you feel about allowing line transformers to perform some layout consistency checks? By that I mean the diff line transformer could check if the CSS variable for left padding is equal to or larger than a minimum value, say 16px. If not the case, it could either issue a warning to the user or just increase it to the minimum required value (in the hopes that this might be less annoying to users).

Taking a step back and reconsidering the need for diff icons, I don't think I've ever consciously noted the little +/- signs in a diff view before starting work on this PR. I feel like the red and green highlighting already provide sufficient visual guidance on what's been added and removed. Even color blind people probably just assume that what's highlighted in a different shade of gray below another line whose content is quite similar constitutes new code that replaces the old above. Not sure there's a strong need for +/- even there.

In general though, I don't see how you could safely insert any line prefixes, be they line numbers or diff icons (+/-) without checking for it in the line transformer itself. It alone knows how much horizontal padding it needs.

[andrewbranch]

How do you feel about allowing line transformers to perform some layout consistency checks? By that I mean the diff line transformer could check if the CSS variable for left padding is equal to or larger than a minimum value, say 16px. If not the case, it could either issue a warning to the user or just increase it to the minimum required value (in the hopes that this might be less annoying to users).

I don’t see how this is possible—you won’t have access to CSS (or DOM functions like getComputedStyle) at build time.

I think it might be worth it to make the gutter a first-class concept. I’m thinking that a line transformer can optionally return some text/html to insert into the gutter, and then if any line from a given line transformer returns non-empty gutter text, we’ll push a preceding span onto every line. By setting them to display: table-cell, we can ensure that the contributions from different line transformers are all aligned, and the overall gutter width will be consistent across every line, but as narrow as possible: https://codepen.io/atcb/pen/ZEbabYL. It still needs a little work, as this display mode breaks the existing method of setting horizontal padding.

I’ll plan to work on this tomorrow and/or over the course of next week in a separate PR. I think that should set us up well for this PR and for line numbers. Thanks for your patience on this!

[janosh]

I don’t see how this is possible—you won’t have access to CSS (or DOM functions like getComputedStyle) at build time.

Oh right, that's a good point. I keep forgetting about this. 🤦

You could maybe make this work with Gatsby's Server Rendering APIs (onPreRenderHTML and onRenderBody) but it would probably be a pain to make those available to line transformers. Even if you manage, post-processing the user's CSS sounds brittle and like a certain path to unhappiness. 😞

I think it might be worth it to make the gutter a first-class concept.

👍

[andrewbranch]

Update: did a big chunk of work on this today, but display: table-cell didn’t work out as well as I thought it would, so need to iterate a little more on the rendering and layout. Because this is going to change the element structure (and so could break people’s CSS), I’m planning on rolling all this into a v3.

[janosh]

Because this is going to change the element structure (and so could break people’s CSS), I’m planning on rolling all this into a v3.

Sounds good. Let me know if there's anything I can help test.

[andrewbranch]

If you want to rebase onto the v3 branch (and by that I mean both git rebase your branch and change the base branch for this PR from master to v3), you should be unblocked! In your line transformer, you’ll want to strip the leading + or - from the result text, and instead return that character in the new gutterCells property. You can use the line number transformer I added as an example:

gatsby-remark-vscode/src/transformers/lineNumberTransformer.js

Lines 31 to 36 in cb774a2

	gutterCells: [
	{
	className: 'grvsc-line-number',
	text: String(lineNumber)
	}
	],

After you rebase, I can take a closer look at your initial questions about accessing the code fence parsedOptions in your line transformer, although the lineNumberTransformer above does that, so maybe that will help. Thanks for your patience as always; I’m excited to finally be able to get this rolling again!

[janosh]

@andrewbranch Very cool! Thanks for all the effort you put into this plugin.

I rebased, changed the base branch to v3 and used the new gutterCells prop in diffLineTransformer. So is meta the 'new' parsedOptions?

[janosh]

@andrewbranch Still need to update the tests but maybe it's good to wait until the code is ready. As always, feel free to step in and make changes to this PR wherever you want.

[andrewbranch]

This is looking great! I think there are just some minor tweaks remaining, plus additional testing. I’d like to use this PR as a way to further test the gutter cells implementation to ensure that multiple line transformers can contribute gutter cells, so it would be great to get a test that combines line numbering and diff highlighting. I also think we should look at the diff colors on a variety of light and dark themes. I’ll plan to update the integration testing infrastructure to allow a single test to be run multiple times with different options, so we can just map over the theme names and generate some output for each.

[andrewbranch]

As always, feel free to step in and make changes to this PR wherever you want.

I took the liberty of updating the tests, fixing the one type error (nice job on the rest of the types by the way), and updating the postfix space requirement locally. But it looks like you don’t have the “allow edits from maintainers” checkbox checked 😁

[janosh]

... and updating the postfix space requirement locally.

Oops, should have read this first.

But it looks like you don’t have the “allow edits from maintainers” checkbox checked 😁

I do have it checked:

I tried unchecking and rechecking just now. Maybe that helps?

[andrewbranch]

I do have it checked:

Weird, I’m still getting permission denied. Maybe it's because the PR is from your master branch? I’ll PR my changes to your remote, then. I think it’s ready to merge after that.

[andrewbranch]

Welp, looks like GitHub is having downtime, so I guess I’m not doing this right now 😐

[andrewbranch]

Ok, after refreshing 100 times I created it: https://github.com/janosh/gatsby-remark-vscode/pull/1

[andrewbranch]

Thanks as always, @janosh! 🌟

[andrewbranch]

Thanks as always, @janosh! 🌟

[janosh]

@andrewbranch My pleasure! Do you have a timeline for v3? Let me know if I can help.

[andrewbranch]

No strict timeline, depends on how long everything takes in #96, but I plan on spending some time on it this weekend. I think the most time-consuming task will be writing good documentation for the GraphQL stuff and line transformers. Finishing this weekend (even with the US holiday on Monday) is probably too optimistic. Probably by the end of next weekend is the best-case scenario.

You can always give gatsby-remark-vscode@alpha a try in the meantime, and let me know if anything listed in #96 sounds like something you want to work on, although most of it is just housekeeping and docs writing at this point.