github.com/shurcooL/issuesapp/...

Add diff highlighting CSS classes. github.com/shurcooL/issuesapp#21

Closeddmitshur opened this issue 8 years ago
dmitshur commented 8 years ago

The rendered markdown already marks up diff fenced code blocks with highlighting (including intra-block character-level highlighting). However, Sourcegraph CSS, which is being used by Tracker, does not include any implementation of those classes.

So a diff fenced code block currently appears plain. E.g.:

diff --git a/app/appconf/appconf.go b/app/appconf/appconf.go
index 6b5113a..6447fab 100644
--- a/app/appconf/appconf.go
+++ b/app/appconf/appconf.go
@@ -58,7 +58,9 @@ var Flags struct {
 
 	CheckForUpdates time.Duration `long:"app.check-for-updates" description:"rate at which to check for updates and display a notification (not download/install) (0 to disable)" default:"30m"`
 
-	Blog bool `long:"app.blog" description:"Enable the Sourcegraph blog, must also set $SG_TUMBLR_API_KEY"`
+	BlogChanged int64 `long:"app.blog" description:"Enable the Sourcegraph blog, must also set $SG_TUMBLR_API_KEY"`
+
+	SomethingAdded bool
 
 	DisableExternalLinks bool `long:"app.disable-external-links" description:"Disable links to external websites"`
 

If we add the CSS for those classes, which are "gi", "gd", "gu", "gh" for outer blocks, "x" for inner emphasis blocks (source), then the above will appear in a more readable highlighted form:

/cc @slimsag @renfredxh

Write Preview Markdown
slimsag commented 8 years ago

I think this is a good idea. One concern I have is: does this mean Tracker would support these nicer diffs but other apps rendering markdown wouldn't? is it handled per-app right now? I think we should make sure that we keep a "Sourcegraph Style Markdown" for example, that remains consistent across all apps.

Write Preview Markdown
dmitshur commented 8 years ago

I agree with that.

This is not specific to Tracker, it is specific to using a specific Markdown renderer (namely, this one).

We certainly can and should expand that to all of Sourcegraph, not just apps.

I used that Markdown renderer in Tracker because I was building this platform app at a later time and had the freedom of picking a Markdown renderer myself. The core Sourcegraph codebase uses another less sophisticated Markdown renderer (a blackfriday.Html renderer with a certain set of options set). It previously used at least 2 different Markdown renderers in the frontend (written in JS).

The end goal future direction is to have a Sourcegraph-specific blackfriday.Renderer IMO. This current existing renderer can be a good starting point. But we need to figure out and agree on the specifics separately.

Write Preview Markdown
slimsag commented 8 years ago

The end goal future direction is to have a Sourcegraph-specific blackfriday.Renderer IMO. This current existing renderer can be a good starting point. But we need to figure out and agree on the specifics separately.

I think this is a very good idea. How about we start migrating in this direction right now, rather than later? We don't have to migrate all uses over to it just yet -- but we could start out by just declaring a "Sourcegraph markdown" in a Go package somewhere -- and making use of it in Tracker to begin with? This way we could continue making improvements to it as we need more changes like this one. What do you think?

Write Preview Markdown
dmitshur commented 8 years ago

Agreed.

How about src.sourcegraph.com/markdown as the import path?

It can have a single method func Render(markdown []byte) (sanitizedHTML []byte) or similar:

// Render renders and sanitizes markdown.
func Render(markdown []byte) (sanitizedHTML []byte) {
	return github_flavored_markdown.Markdown(markdown)
}

That could be the initial implementation; we can make it more custom and elaborate (i.e., bring in actual source of github_flavored_markdown package and be able to make changes to it) as our needs increase and time budget allows.

Write Preview Markdown
renfredxh commented 8 years ago

Agreed. We'll need to add support for some custom markdown syntax (@ mentions, emoji, etc.) eventually, so would be a good idea to start making this possible now.

Write Preview Markdown
dmitshur commented 7 years ago

The original issue has been resolved, and diffs are rendered nicely now. It uses the relevant CSS from Primer.

It was done in https://github.com/shurcooL/issuesapp/commit/3c28bdba09fcf83b7cc784c7428d750b6b98f903.

Write Preview Markdown
dmitshur closed this 7 years ago
to comment.