GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: adds doc links to the stability index reference by michaelcox · Pull Request #11664 · nodejs/node

Conversation

@michaelcox

@michaelcox michaelcox commented Mar 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

Description of Change

This modifies the script that generates the docs to create a static link from each Stability Index callout bar to the Stability Index explanations in documentation.html. This was a first-commit request from @Trott. Here is what he pitched to me:

Many APIs have a Stability Index associated with them. Check out https://nodejs.org/dist/latest-v7.x/docs/api/modules.html#modules_modules for an example (the Modules API is Locked, Stability Index 3).
It would be great if there could be a link in the Stability Index banner to the definition of the various Stability Indexes. If I don't know what "Locked" means, then I can click the link and find out.
Modify html.js to include such a link. This will involve some HTML of course, and probably some CSS and maybe some back-and-forth about usability/styling once the PR is open.

I looked for any precedent for styling, and the best I could find is that when features are deprecated, a simple link is put to the new API to use, inside the banner. This similarly just links the first part (e.g. Stability: 2) to the documentation.html page. I'm obviously open to other styling suggestions, but this seemed the most straightforward. There doesn't seem to be any precedent in the docs for other things like a ? icon or similar.

Also I'm just linking to the main anchor for the Stability Index explanations, rather than to a specific one. My thinking here is that there are only a handful of them, and they should all fit nicely above the fold for anyone reading that page. It didn't seem necessary to create additional anchors.

Note that I had to make a couple more variables in the commit here to make the tests pass with the 80 character line limit.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 2, 2017
@hiroppy

hiroppy commented Mar 2, 2017

Copy link
Copy Markdown
Member

I think that the commit message should be tools rather than doc;)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@michaelcox

michaelcox commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Thanks - updated to be tools instead. Makes more sense. :)

hiroppy reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@michaelcox michaelcox changed the title doc: adds links to the stability index reference tools: adds doc links to the stability index reference Mar 3, 2017
@addaleax addaleax added the doc Issues and PRs related to Node.js documentation. label Mar 3, 2017
@hiroppy

hiroppy commented Mar 3, 2017

Copy link
Copy Markdown
Member

cc @nodejs/documentation

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott

Trott commented Mar 3, 2017

Copy link
Copy Markdown
Member

I don't think this file is exercised by any tests, but it is covered by lint rules, so here's a CI lint run for it: https://ci.nodejs.org/job/node-test-linter/7386/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thefourtheye
thefourtheye approved these changes Mar 3, 2017
View reviewed changes
Comment thread tools/doc/html.js Outdated

@thefourtheye thefourtheye Mar 3, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Nit: I would prefer writing this as,

`<pre class="${classNames}">
    <a href="${docsUrl}">$1 $2</a>$3
 </pre>`

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@michaelcox michaelcox Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I've updated to bring the closing </pre> to the same line, but I can't write it nested the way you have it because of the default formatting of a <pre> tag. Doing it the way you had it adds another 20px-ish left padding into the bar.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gibfahn reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@michaelcox
tools: adds links to the stability index reference
412f1e6
This modifies the script that generates the docs to create a
static link from each Stability Index callout bar to the
Stability Index explanations in documentation.html.
jasnell
jasnell approved these changes Mar 3, 2017
View reviewed changes
fhinkel
fhinkel approved these changes Mar 9, 2017
View reviewed changes
@fhinkel

fhinkel commented Mar 9, 2017

Copy link
Copy Markdown
Contributor

Thanks. Landed in dd76f5f

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhinkel fhinkel closed this Mar 9, 2017
fhinkel pushed a commit that referenced this pull request Mar 9, 2017
@michaelcox @fhinkel
tools: add links to the stability index reference
dd76f5f
This modifies the script that generates the docs
to create a static link from each Stability Index callout bar to
the Stability Index explanations in documentation.html.

PR-URL: #11664
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
@michaelcox Italo A. Casas
tools: add links to the stability index reference
5e7baa5
This modifies the script that generates the docs
to create a static link from each Stability Index callout bar to
the Stability Index explanations in documentation.html.

PR-URL: nodejs#11664
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 13, 2017
7.7.3 Release Proposal #11831
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@michaelcox @jungx098
tools: add links to the stability index reference
23f24da
This modifies the script that generates the docs
to create a static link from each Stability Index callout bar to
the Stability Index explanations in documentation.html.

PR-URL: nodejs#11664
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@michaelcox @MylesBorins
tools: add links to the stability index reference
5c00610
This modifies the script that generates the docs
to create a static link from each Stability Index callout bar to
the Stability Index explanations in documentation.html.

PR-URL: #11664
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins added the land-on-v6.x label Apr 17, 2017
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@michaelcox @MylesBorins
tools: add links to the stability index reference
3858861
This modifies the script that generates the docs
to create a static link from each Stability Index callout bar to
the Stability Index explanations in documentation.html.

PR-URL: #11664
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
v6.10.3 proposal #12498
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@michaelcox @andrew749
tools: add links to the stability index reference
81fb65a
This modifies the script that generates the docs
to create a static link from each Stability Index callout bar to
the Stability Index explanations in documentation.html.

PR-URL: nodejs/node#11664
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
+2 more reviewers
@fhinkel fhinkel fhinkel approved these changes
@thefourtheye thefourtheye thefourtheye approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@michaelcox @hiroppy @Trott @fhinkel @jasnell @thefourtheye @MylesBorins @addaleax @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.