doc: Highlight deprecated API components in "Table of Contents" #7189

ilfroloff · 2016-06-07T09:50:34Z

Hi All!

Issue rolled up from nodejs/nodejs.org#772 topic.

P.S. I didn't change the color to grey as it's a bit unnoticeable when used with the green color of items.

stevemao · 2016-06-07T10:42:12Z

cc @nodejs/documentation @nodejs/website

benjamingr · 2016-06-07T13:50:03Z

tools/doc/html.js

    if (tok.type === 'code' && tok.text.match(/Stability:.*/g)) {
+      if(heading && (index - 1 === headingIndex || index - 2 === headingIndex)) {
+        heading.stability =  +tok.text.match(/.*:\s(\d)[\s\S]*/)[1];


Is this really the cleanest way to do this? Looks super hacky.

Yep, it's one of the cleanest solutions in this situation. I got regular expression from parseAPIHeader method which used below in code.
Code: https://github.com/nodejs/node/blob/master/tools/doc/html.js#L265

Note about (index - 1 === headingIndex || index - 2 === headingIndex):
I didn't find better solution for this moment.
It's small hack for detection place for marking stability in heading block using tok.type === 'code'.

Fishrock123 · 2016-06-07T14:41:33Z

/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:48: trailing whitespace.

/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:54: trailing whitespace.

/Users/Jeremiah/Documents/node/.git/rebase-apply/patch:124: trailing whitespace.

warning: 3 lines applied after fixing whitespace errors.

Could you please set git config --global --add core.whitespace fix? thanks!

Fishrock123 · 2016-06-15T22:46:03Z

Seems to work. I like it.

I was a bit conflicted at first about the strike through but I think that may be good for colorblind users.

cc @nodejs/documentation

Fishrock123 · 2016-06-15T22:47:13Z

tools/doc/html.js

-  var output = [];
+  const output = [];
+  let state = null;
+  let depth = 0;


Unnecessary changes?

Fishrock123 · 2016-06-15T22:49:03Z

@ilfroloff I'll be willing to review more if you could change less where not necessary. :)

Fishrock123 · 2016-06-15T22:52:23Z

Also if you could run make -j4 lint that would be great!

ilfroloff · 2016-06-16T08:21:33Z

Thanks for answer.
I'll revert all unnecessary change. Sorry for this

ilfroloff · 2016-06-16T09:17:40Z

@Fishrock123 I removed unnecessary changes and resolved error from make -j4 lint command.

Fishrock123 · 2016-06-16T14:07:33Z

tools/doc/html.js

    if (tok.type === 'code' && tok.text.match(/Stability:.*/g)) {
-      tok.text = parseAPIHeader(tok.text);
-      output.push({ type: 'html', text: tok.text });
+      const stabilityTextRegExp = /(.*:)\s(\d)([\s\S]*)/;


can probably be a top-level variable, that way we don't need to pass it to parseAPIHeader

Fishrock123 · 2016-06-17T20:43:33Z

LGTM minus comments

Fishrock123 · 2016-06-30T12:41:57Z

ping @ilfroloff

ilfroloff · 2016-06-30T17:32:38Z

@Fishrock123 pong)
I'll push corrections in soon.

ilfroloff · 2016-07-05T05:41:06Z

@Fishrock123 completed. I implemented your suggestions

Fishrock123 · 2016-07-18T09:51:53Z

tools/doc/html.js

    toc.push(new Array((depth - 1) * 2 + 1).join(' ') +
-             '* <a href="#' + id + '">' +
-             tok.text + '</a>');
+             '* %stability_' + tok.stability + '%' +


@ilfroloff Would this be better as

var toc_entry = tok.stability ? `<li class="${tok.stability}">` : '<li>'; toc_entry += `<a href="#' + id + '">${tok.text}</a>`; toc.push(new Array((depth - 1) * 2 + 1).join(' ') + toc_entry);

That would get rid of the massive find/replace after the fact?

@Fishrock123 sorry, but this code cannot be implemented, because <li> html tag generated by this code.

Line 334

+ '* %stability_' + tok.stability + '%' +

need because I want to add class attribute for <li> after parsing.

Also, <li> don't need to be closed, because </li> tag generates by markdown parser.

Fishrock123 · 2016-07-18T09:53:59Z

@ilfroloff It doesn't look like the <li>'s are ever closed with a </li>?

Fishrock123 · 2016-08-02T14:17:47Z

ping @ilfroloff

Fishrock123 · 2016-09-02T15:06:55Z

@ilfroloff just tried to rebase and it didn't seem to work...

If you have time could rebase? Also, I still don't understand why my find/replace comment wouldn't work.. :(

If you don;t, mind if I take this PR over and make a new one (preserving your authorship)? I'd like to see something like this land!

ilfroloff · 2016-09-02T21:37:24Z

@Fishrock123 sorry, I didn't see that PR couldn't be rebased without conflicts. I fetched current master branch and adapted my code to new source)

Also, I still don't understand why my find/replace comment wouldn't work.. :(

new Array((depth - 1) * 2 + 1).join(' ')

This part works only with * because it provides intent of markdown's list. But you replaced * to <li> tag and markdown's parser built incorrect html.

In general, reason of adding was to set "class" attribute in <li> tag for restyling all <a> children. But after your question, I rethought and remade code without find/replace of <li>. Now, "class" attribute is set directly to <a> tag. You can check it in update. Thanks for advice)

Only one difference appeared after removing find/replace (it was one of reason why I added find/replace):

Also similar in punycode module.

Thanks for answers.

silverwind · 2016-09-03T04:11:18Z

That strike-through style might be a bit too much imho, it conveys to me that the component has been removed, which isn't true. How about just adding a tag like GitHub issues have? Could be done through an ::after element.

silverwind · 2016-09-03T04:18:19Z

How about something like this?

#toc .stability_0:after {
  content: "deprecated";
  font-size: .8em;
  position: relative;
  top: -.1em;
  margin-left: .3em;
  background: #f44;
  color: #fff;
  padding: .1em .3em;
  border-radius: 3px;
}

lpinca · 2016-10-09T13:13:45Z

This is how it looks for domain and punycode:

silverwind · 2016-10-09T13:28:29Z

The only concern I have with this is that the red link might give the impression that the target is not there. (like red links on Wikipedia).

lpinca · 2016-10-09T13:33:38Z

@silverwind I agree, read my comment above, I prefer to only have the label and leave the link color unchanged as you originally suggested. It's slick and effective imo.

ilfroloff · 2016-10-10T08:16:42Z

@lpinca

I prefer to only have the label and leave the link color unchanged as you originally suggested

Yep, I have changed deprecated color to original green. It looks like:

Also, one of reason, I have had the problem how to change color to red for all domain or punycode children links - it's pretty hard in current building of docs html.

lpinca · 2016-10-10T08:24:39Z

@ilfroloff awesome, I love that. Thanks!

silverwind · 2016-10-10T17:04:25Z

Can anyone test this? I just tried and can't see a difference. No classes seem to have been added.

lpinca · 2016-10-10T17:35:51Z

@silverwind works for me.

silverwind · 2016-10-10T18:02:39Z

Hmm strange, must be something about my setup then. I'm doing

apply-pr 7189 && ./configure && make doc-only && http-server out/doc/api

lpinca · 2016-10-10T18:07:46Z

@silverwind that's what I did as well

curl -L https://github.com/nodejs/node/pull/7189.patch | git am
make doc-only

and opened the html files with the browser

silverwind · 2016-10-10T18:19:32Z

Ah, was a bad build cache, rm -rf out/doc did help.

silverwind

LGTM besides centering nit.

silverwind · 2016-10-10T18:23:03Z

doc/api_assets/style.css

+  content: "deprecated";
+  font-size: .8em;
+  position: relative;
+  top: -.1em;


Not sure why (em cascade?), but -.18em seems to center it better:

Yep, agreed. I have changed top to .18em

Highlight deprecated API methods/prorepties in "Table of Contents" for increasing understandable Adapted code to eslint standarts

silverwind · 2016-10-11T22:57:28Z

@lpinca lgty?

lpinca · 2016-10-12T05:44:57Z

@silverwind yes, I didn't sign off because I'm not very familiar with the doc build tools.

thefourtheye · 2016-10-11T16:39:27Z

tools/doc/html.js

@@ -9,6 +9,8 @@ const typeParser = require('./type-parser.js');

 module.exports = toHTML;

+const STABILITY_TEXT_REG_EXP = /(.*:)\s(\d)([\s\S]*)/;


Would it be better to make the \s greedy, which is before the digits?

silverwind · 2016-10-12T14:38:05Z

tools/doc/html.js

@@ -167,6 +172,17 @@ function parseLists(input) {
    if ((tok.type === 'paragraph' && state === 'MAYBE_STABILITY_BQ') ||
      tok.type === 'code') {
      if (tok.text.match(/Stability:.*/g)) {
+        const stabilityMatch = tok.text.match(STABILITY_TEXT_REG_EXP);
+        const stability = +stabilityMatch[2];


Nit: Number(stabilityMatch[2]) might be cleaner.

Highlight deprecated API methods/properties in "Table of Contents" for increasing understandability. Adapted code to eslint standards. PR-URL: #7189 Fixes: nodejs/nodejs.org#772 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

silverwind · 2016-10-14T05:34:46Z

Landed with greedy regex and number constructor in fcee4d4, thanks!

Highlight deprecated API methods/properties in "Table of Contents" for increasing understandability. Adapted code to eslint standards. PR-URL: #7189 Fixes: nodejs/nodejs.org#772 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

ilfroloff mentioned this pull request Jun 7, 2016

Highlight deprecated API components in "Table of Contents" nodejs/nodejs.org#772

Closed

ilfroloff changed the title ~~docs: Highlight deprecated API components in "Table of Contents"~~ doc: Highlight deprecated API components in "Table of Contents" Jun 7, 2016

mscdex added the doc Issues and PRs related to the documentations. label Jun 7, 2016

benjamingr reviewed Jun 7, 2016
View reviewed changes

Fishrock123 reviewed Jun 15, 2016
View reviewed changes

tools/doc/html.js

var output = [];

const output = [];

let state = null;

let depth = 0;

Copy link

Contributor

Fishrock123 Jun 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary changes?

Fishrock123 reviewed Jun 16, 2016
View reviewed changes

Fishrock123 self-assigned this Jun 30, 2016

Fishrock123 reviewed Jul 18, 2016
View reviewed changes

Fishrock123 approved these changes Oct 10, 2016

View reviewed changes

silverwind suggested changes Oct 10, 2016

View reviewed changes

docs: highlight deprecated API in ToC

fd9d2aa

Highlight deprecated API methods/prorepties in "Table of Contents" for increasing understandable Adapted code to eslint standarts

silverwind approved these changes Oct 10, 2016

View reviewed changes

ghost approved these changes Oct 10, 2016

View reviewed changes

Fishrock123 approved these changes Oct 11, 2016

View reviewed changes

thefourtheye approved these changes Oct 12, 2016

View reviewed changes

silverwind reviewed Oct 12, 2016

View reviewed changes

silverwind closed this Oct 14, 2016

MylesBorins added land-on-v4.x labels Nov 11, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

		@@ -9,6 +9,8 @@ const typeParser = require('./type-parser.js');

		module.exports = toHTML;

		const STABILITY_TEXT_REG_EXP = /(.:)\s(\d)([\s\S])/;

doc: Highlight deprecated API components in "Table of Contents" #7189

doc: Highlight deprecated API components in "Table of Contents" #7189

Conversation

ilfroloff commented Jun 7, 2016

stevemao commented Jun 7, 2016 • edited Loading

benjamingr Jun 7, 2016

Choose a reason for hiding this comment

ilfroloff Jun 7, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 commented Jun 7, 2016

Fishrock123 commented Jun 15, 2016

Fishrock123 Jun 15, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jun 15, 2016

Fishrock123 commented Jun 15, 2016

ilfroloff commented Jun 16, 2016

ilfroloff commented Jun 16, 2016

Fishrock123 Jun 16, 2016

Choose a reason for hiding this comment

Fishrock123 commented Jun 17, 2016

Fishrock123 commented Jun 30, 2016

ilfroloff commented Jun 30, 2016

ilfroloff commented Jul 5, 2016

Fishrock123 Jul 18, 2016

Choose a reason for hiding this comment

ilfroloff Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

Fishrock123 commented Jul 18, 2016

Fishrock123 commented Aug 2, 2016

Fishrock123 commented Sep 2, 2016

ilfroloff commented Sep 2, 2016

silverwind commented Sep 3, 2016

silverwind commented Sep 3, 2016

lpinca commented Oct 9, 2016

silverwind commented Oct 9, 2016 • edited Loading

lpinca commented Oct 9, 2016 • edited Loading

ilfroloff commented Oct 10, 2016

lpinca commented Oct 10, 2016

silverwind commented Oct 10, 2016

lpinca commented Oct 10, 2016

silverwind commented Oct 10, 2016

lpinca commented Oct 10, 2016

silverwind commented Oct 10, 2016

silverwind left a comment

Choose a reason for hiding this comment

silverwind Oct 10, 2016

Choose a reason for hiding this comment

ilfroloff Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

silverwind commented Oct 11, 2016

lpinca commented Oct 12, 2016

thefourtheye Oct 11, 2016

Choose a reason for hiding this comment

silverwind Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

silverwind commented Oct 14, 2016

stevemao commented Jun 7, 2016 •

edited

Loading

ilfroloff Jun 7, 2016 •

edited

Loading

ilfroloff Aug 2, 2016 •

edited

Loading

silverwind commented Oct 9, 2016 •

edited

Loading

lpinca commented Oct 9, 2016 •

edited

Loading

ilfroloff Oct 10, 2016 •

edited

Loading

silverwind Oct 12, 2016 •

edited

Loading