doc: Highlight deprecated API components in "Table of Contents" by ilfroloff · Pull Request #7189 · nodejs/node · GitHub

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.

cc @nodejs/documentation @nodejs/website

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'.

/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!

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

Unnecessary changes?

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

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

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

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

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

LGTM minus comments

ping @ilfroloff

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

@Fishrock123 completed. I implemented your suggestions

@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.

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

ping @ilfroloff

@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!

@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.

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.

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;
}

This is how it looks for domain and punycode:

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).

@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.

@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.

@ilfroloff awesome, I love that. Thanks!

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

@silverwind works for me.

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

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

@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

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

LGTM besides centering nit.

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

@lpinca lgty?

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

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

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>

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

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

                     tok.type === 'code') {
                     if (tok.text.match(/Stability:.*/g)) {
+                      const stabilityMatch = tok.text.match(STABILITY_TEXT_REG_EXP);
+                      const stability = +stabilityMatch[2];


		module.exports = toHTML;

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

doc: Highlight deprecated API components in "Table of Contents" by ilfroloff · Pull Request #7189 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

doc: Highlight deprecated API components in "Table of Contents"#7189ilfroloff wants to merge 1 commit intonodejs:masternodejs/node:masterfrom ilfroloff:docs-deprecated-highlightCopy head branch name to clipboard

Conversation

ilfroloff commented Jun 7, 2016

Uh oh!

stevemao commented Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjamingr Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

ilfroloff Jun 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jun 7, 2016

Uh oh!

Fishrock123 commented Jun 15, 2016

Uh oh!

Fishrock123 Jun 15, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jun 15, 2016

Uh oh!

Fishrock123 commented Jun 15, 2016

Uh oh!

ilfroloff commented Jun 16, 2016

Uh oh!

ilfroloff commented Jun 16, 2016

Uh oh!

Fishrock123 Jun 16, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jun 17, 2016

Uh oh!

Fishrock123 commented Jun 30, 2016

Uh oh!

ilfroloff commented Jun 30, 2016

Uh oh!

ilfroloff commented Jul 5, 2016

Uh oh!

Fishrock123 Jul 18, 2016

Choose a reason for hiding this comment

Uh oh!

ilfroloff Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jul 18, 2016

Uh oh!

Fishrock123 commented Aug 2, 2016

Uh oh!

Fishrock123 commented Sep 2, 2016

Uh oh!

ilfroloff commented Sep 2, 2016

Uh oh!

silverwind commented Sep 3, 2016

Uh oh!

silverwind commented Sep 3, 2016

Uh oh!

lpinca commented Oct 9, 2016

Uh oh!

silverwind commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilfroloff commented Oct 10, 2016

Uh oh!

lpinca commented Oct 10, 2016

Uh oh!

silverwind commented Oct 10, 2016

Uh oh!

lpinca commented Oct 10, 2016

Uh oh!

silverwind commented Oct 10, 2016

doc: Highlight deprecated API components in "Table of Contents"#7189
ilfroloff wants to merge 1 commit into
nodejs:masterfrom
ilfroloff:docs-deprecated-highlight

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