Use highlight js #1489

kevmoo · 2017-08-31T02:05:59Z

No description provided.

Aligning with similar change in dartdoc: dart-lang/dartdoc#1489

kevmoo · 2017-08-31T15:48:44Z

FYI: just pushed an `oops' commit that adds swift, obj-c, and java (for Flutter interop packages)

Will cleanup/rebase once approved

kevmoo · 2017-08-31T20:17:23Z

Thoughts @jcollins-g and @devoncarew ?

Aligning with similar change in dartdoc: dart-lang/dartdoc#1489

jcollins-g

This change could use at least some screenshots demonstrating the change in appearance, but ideally a navigable example of something complicated like the SDK serving somewhere so we can see the new look and feel.

jcollins-g · 2017-09-12T14:23:49Z

lib/resources/github.css

@@ -0,0 +1,99 @@
+/*
+
+github.com style (c) Vasily Polovnyov <[email protected]>


Please indicate somewhere in LICENSE/README where this and other new third-party libraries included come from (including URLs) and their licensing.

jcollins-g · 2017-09-12T14:29:39Z

lib/src/markdown_processor.dart

@@ -797,7 +797,6 @@ class MarkdownDocument extends md.Document {
      }

      bool specifiesLanguage = pre.classes.isNotEmpty;
-      pre.classes.add('prettyprint');


Could you mention a bit more about how this is replaced?

prettyprint was used by the pretty print library to find elements to style at runtime.

highlight.js looks at all code blocks with language-XYZ – no prettyprint needed

jcollins-g · 2017-09-12T14:30:12Z

tool/travis.sh

@@ -57,5 +57,5 @@ else
  (cd testing/test_package_small; dart -c ../../bin/dartdoc.dart)

  # Run the tests.
-  pub run grinder test
+  pub run test


Why the change?

See comments on this commit: 90a379d

jcollins-g · 2017-09-12T14:30:56Z

tool/grind.dart

-  // using straight-up VM here
-  return Dart.runAsync('test/all.dart', vmArgs: ['--checked']);
+  // `pub run test` is a bit slower than running an `test_all.dart` script
+  // But it provides more useful output in the case of failures.


Could you be more specific?

See comments on this commit: 90a379d

jcollins-g · 2017-09-12T14:35:02Z

testing/test_package/lib/fake.dart

@@ -496,7 +496,7 @@ class BaseForDocComments {
  ///
  /// Reference to a top-level const in another library [incorrectDocReferenceFromEx]
  ///
-  /// Reference to prefixed-name from another lib [css.theOnlyThingInTheLibrary] xx
+  /// Reference to prefixed-name from another lib [example_prefixed.incorrectDocReferenceFromEx]


Does css.theOnlyThingInTheLibrary still fail?

That member was removed – but since this test was skipped, it didn't change output.

Just picked something that already existed.

The member isn't actually gone, so this test should pass (assuming we didn't have the bug it is testing for).

kevmoo · 2017-09-12T16:45:20Z

@jcollins-g here's the before/after

For the original bug, the new style is much more clean about comments vs ID selectors.

Before

After

As is tradition

Fixes #1484

It may be a bit slower, but * It provides more helpful errors on failure * It captures all declared tests * `all.dart` was missing `utils_test.dart` for example

kevmoo · 2017-09-12T17:24:27Z

Sorry. Had to do a rebase fix – but went back to the original

…

On Tue, Sep 12, 2017 at 10:11 AM, jcollins-g ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In testing/test_package/lib/fake.dart <#1489 (comment)>: > @@ -496,7 +496,7 @@ class BaseForDocComments { /// /// Reference to a top-level const in another library [incorrectDocReferenceFromEx] /// - /// Reference to prefixed-name from another lib [css.theOnlyThingInTheLibrary] xx + /// Reference to prefixed-name from another lib [example_prefixed.incorrectDocReferenceFromEx] The member isn't actually gone, so this test should pass (assuming we didn't have the bug it is testing for). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1489 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCip3iit6kO4HaXJLfBLi_z8vNN7Rpks5shrtQgaJpZM4PIM5q> .

jcollins-g

Minor nit. Otherwise, looks good and the screenshots definitely show the improvement here. Thanks!

jcollins-g · 2017-09-12T17:32:53Z

test/model_test.dart

-            docsAsHtml, contains('<code>css.theOnlyThingInTheLibrary</code>'));
+            docsAsHtml,
+            contains(
+                '<p>Reference to prefixed-name from another lib <code>css.theOnlyThingInTheLibrary</code> xx</p>'));


Now this has " xx" on the end?

kevmoo · 2017-09-12T18:13:19Z

@jcollins-g – FYI. I removed the fixed test.

In the test case, the link is still broken....but in the full output, the link is there.

Zero clue why.

kevmoo requested review from devoncarew and jcollins-g August 31, 2017 02:05

googlebot added the cla: yes Google CLA check succeeded. label Aug 31, 2017

kevmoo mentioned this pull request Aug 31, 2017

With language-css, # shouldn't be treated as a comment #1484

Closed

kevmoo mentioned this pull request Aug 31, 2017

Update highlight.js languages dart-lang/pub-dev#325

Merged

kevmoo force-pushed the use_highlight_js branch 3 times, most recently from 3bc7986 to 8dee2ba Compare August 31, 2017 04:03

kevmoo added a commit to dart-lang/pub-dev that referenced this pull request Aug 31, 2017

Update highlight.js

44104f5

Aligning with similar change in dartdoc: dart-lang/dartdoc#1489

mkustermann pushed a commit to dart-lang/pub-dev that referenced this pull request Sep 1, 2017

Update highlight.js (#325)

76f753a

Aligning with similar change in dartdoc: dart-lang/dartdoc#1489

kevmoo force-pushed the use_highlight_js branch from 0ddecd2 to a589c97 Compare September 7, 2017 04:44

jcollins-g suggested changes Sep 12, 2017

View reviewed changes

kevmoo added 4 commits September 12, 2017 10:17

Use setupAll in model_test

54c797c

As is tradition

test_package: Add css, html, and js examples to readme

7a35193

Use highlight.js instead of prettify

541e01b

Fixes #1484

Use pub run test for testing

2029555

It may be a bit slower, but * It provides more helpful errors on failure * It captures all declared tests * `all.dart` was missing `utils_test.dart` for example

kevmoo force-pushed the use_highlight_js branch from a589c97 to dd3d83d Compare September 12, 2017 17:24

jcollins-g approved these changes Sep 12, 2017

View reviewed changes

dartfmt

82390a1

kevmoo force-pushed the use_highlight_js branch from dd3d83d to 82390a1 Compare September 12, 2017 18:07

kevmoo merged commit 82390a1 into master Sep 12, 2017

kevmoo deleted the use_highlight_js branch September 12, 2017 18:08

jcollins-g mentioned this pull request Oct 24, 2017

Included source code no longer highlights #1525

Closed

		@@ -0,0 +1,99 @@
		/*

		github.com style (c) Vasily Polovnyov <[email protected]>

Use highlight js #1489

Use highlight js #1489

Uh oh!

Conversation

kevmoo commented Aug 31, 2017

Uh oh!

kevmoo commented Aug 31, 2017

Uh oh!

kevmoo commented Aug 31, 2017

Uh oh!

jcollins-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevmoo commented Sep 12, 2017

Uh oh!

kevmoo commented Sep 12, 2017 via email

Uh oh!

jcollins-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevmoo commented Sep 12, 2017

Uh oh!

Uh oh!