Syntax highlighting for SDK docs

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: wbamberg, Assigned: wbamberg)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
It would help readability if we implemented syntax highlighting for code samples in the SDK.
(Assignee)

Comment 1

8 years ago
Created attachment 496669 [details]
Links to Git branch containing syntax highlighting

I tried implementing syntax highlighting for the code examples in the SDK. It uses Google's syntax highlighter (http://code.google.com/p/syntaxhighlighter/) and will try to highlight any <pre><code> blocks.

I copied the license (Apache license) to the same place as the JS and CSS files themselves.

I've gone through any Markdown-style code blocks that aren't actual code samples and turned them into explicit <pre> blocks, so they don't get highlighted too.

If you like this approach, let me know, and I'll make a pull request.
Attachment #496669 - Flags: feedback?(myk)
Comment on attachment 496669 [details]
Links to Git branch containing syntax highlighting

I like this approach!  And I think the Apache 2.0 license is compatible with our tri-license.  But I've emailed the Mozilla licensing folks to confirm that.


In the process of giving feedback, I ended up doing a review of the code, since I was reading through it anyway, and I noticed one issue:

> +    $("pre>code").addClass("prettyprint");

This adds class "prettyprint" to <code> elements that are children of <pre> elements.  But prettify.css expects the class to be placed on the <pre> elements themselves:

> +pre.prettyprint { padding: 2px; border: 1px solid #888 }

We could modify prettify.css to style code.prettyprint instead, but that makes it harder to merge upstream changes, so we should probably do something like modify our HTML generation to do <pre class="code"> instead of <pre><code> and then $("pre.code").addClass("prettyprint").

(Ideally, we'd be able to select <pre> elements with a <code> child, but though it's possible in XPath, as far as I know it isn't possible in CSS and hence also not in jQuery.)


> +.str { color: #080; }

I wish these were restricted to pretty-printed sections, i.e.:

  pre.prettyprint .str { color: #080; }

But it's probably better to leave them as-is to make it easier to merge from upstream, especially since they probably don't trod on any existing or planned styles.  Or perhaps we could push this change upstream (provided it doesn't incur a significant performance cost).
Attachment #496669 - Flags: review-
Attachment #496669 - Flags: feedback?(myk)
Attachment #496669 - Flags: feedback+
(Assignee)

Comment 3

8 years ago
Thanks Myk!

(In reply to comment #2)
> 
> In the process of giving feedback, I ended up doing a review of the code, since
> I was reading through it anyway, and I noticed one issue:
> 
> > +    $("pre>code").addClass("prettyprint");
> 
> This adds class "prettyprint" to <code> elements that are children of <pre>
> elements.  But prettify.css expects the class to be placed on the <pre>
> elements themselves:
> 
> > +pre.prettyprint { padding: 2px; border: 1px solid #888 }

True. It does mostly work when the class is on <code> but you don't get the border.

> We could modify prettify.css to style code.prettyprint instead, but that makes
> it harder to merge upstream changes, so we should probably do something like
> modify our HTML generation to do <pre class="code"> instead of <pre><code> and
> then $("pre.code").addClass("prettyprint").

Similarly, though: our HTML generation is currently using the Markdown conveter and I'd prefer not to add custom behaviour there either.

> (Ideally, we'd be able to select <pre> elements with a <code> child, but though
> it's possible in XPath, as far as I know it isn't possible in CSS and hence
> also not in jQuery.)

I'm not an expert on JQuery by any stretch, but it looks as if:

$("code").parent("pre").addClass("prettyprint");

works here... or am I missing something?

> > +.str { color: #080; }
> 
> I wish these were restricted to pretty-printed sections, i.e.:
> 
>   pre.prettyprint .str { color: #080; }
> 
> But it's probably better to leave them as-is to make it easier to merge from
> upstream, especially since they probably don't trod on any existing or planned
> styles.  Or perhaps we could push this change upstream (provided it doesn't
> incur a significant performance cost).

Good point. I think it's worth raising an issue with the prettify maintainer for this, and will do that.
(Assignee)

Comment 4

8 years ago
By the way, the link in comment 1 is wrong, it should be: http://code.google.com/p/google-code-prettify/ . Sorry. Everything else is right, including the Apache license.
(Assignee)

Comment 5

8 years ago
Created attachment 499353 [details] [diff] [review]
Updated patch using syntaxhighlighter instead of prettify

I've updated the code to use syntaxhighlighter (http://alexgorbatchev.com/SyntaxHighlighter/) instead, which is available under the MIT license and is, I think, already used in the MDN. I think it looks nicer too, and has support for themes if we care about that.
Attachment #496669 - Attachment is obsolete: true
Attachment #499353 - Flags: review?(myk)
Comment on attachment 499353 [details] [diff] [review]
Updated patch using syntaxhighlighter instead of prettify

(In reply to comment #3)
> I'm not an expert on JQuery by any stretch, but it looks as if:
> 
> $("code").parent("pre").addClass("prettyprint");
> 
> works here... or am I missing something?

Nope, you're not missing anything.  That was my mistake.  It's awesome that jQuery can do this!  (I talked to dbaron about adding it to the CSS selector syntax at one point, and apparently it has tricky performance implications.)


> +  function highlight() {
> +    $("code").parent("pre").addClass("brush: js");

Hunh, what a strange class name.  Technically, this adds two classes, "brush:" and "js", since HTML parsers split class attributes on whitespace (although they also make the entire string available, which must be how syntaxhighlighter accesses it).


> +<pre>
>      python --version
> +</pre>
>  
>  `cfx` currently expects Python 2.5 or 2.6.  Older and newer versions may or may
>  not work.
> @@ -88,7 +90,9 @@ Run the SDK's Unit Tests
>  The SDK comes with a suite of tests which ensures that its APIs work correctly.
>  You can run it with the following command:
>  
> +<pre>
>      cfx testall
> +</pre>
>  
>  Some of the tests will open Firefox windows to check APIs related to the user
>  interface, so don't be alarmed.  Please let the suite finish before resuming
> @@ -97,11 +101,12 @@ your work.
>  When the suite is finished, your text console should contain output that looks
>  something like this:
>  
> +<pre>
>      Testing cfx...

In most cases, you change the indent for explicit <pre> sections from four to two spaces, but here (and in a few other places) you don't.  Is there a reason to leave them with four-space indent?
Attachment #499353 - Flags: review?(myk) → review+
(Assignee)

Comment 7

8 years ago
> In most cases, you change the indent for explicit <pre> sections from four to
> two spaces, but here (and in a few other places) you don't.  Is there a reason
> to leave them with four-space indent?

No good reason. I changed it from 4 to 2 only because the reason for having 4 is to indicate to Markdown that it should use <pre><code>, and changing it to 2 seemed to me to help emphasize that we don't want <code> any more. But that in itself is a bit weak really. Anyway, I've updated it so it's consistently 2 everywhere (I think).

Fixed by: https://github.com/mozilla/addon-sdk/commit/ec07f2a5c874455edb2d44d3362af376e5bdc51e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.