doc: add "Edit on GitHub" link #21703

Trott · 2018-07-07T22:36:50Z

Proof of concept for an "Edit on GitHub" link, inspired by the
Serverless docs.

One issue is that the link is to the version of the docs on the master
branch even if the person was reading a different version of the doc. I
don't consider that a big problem, although we can always remove the
link if it turns out to be a big problem. I don't think there is a good
solution. PRs need to be opened against the master branch generally.
Having a bunch of PRs against staging branches is probably not what we
want. If there's an update to one version of the doc, there will usually
be an update to other versions.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2018-07-07T22:36:51Z

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/224/pipeline

Trott · 2018-07-07T22:37:04Z

@nodejs/website

Trott · 2018-07-07T22:37:58Z

SVG should be in its own file.

CSS should be pulled out of the style attribute and into a CSS file.

If someone's motivated to do that, by all means, push right to the branch to update.

Trott · 2018-07-07T22:38:09Z

@nodejs/documentation

vsemozhetbyt · 2018-07-07T23:01:01Z

The only issue is the all.html: it refers to the https://github.com/nodejs/node/edit/master/doc/api/_toc.md

vsemozhetbyt · 2018-07-07T23:05:15Z

It seems this part of the newborn allhtml.js needs to be updated:

node/tools/doc/allhtml.js

Lines 50 to 54 in 01eee53

    
           // Replace various mentions of _toc with all. 
        
           let all = toc.replace(/_toc\.html/g, 'all.html') 
        
             .replace('_toc.json', 'all.json') 
        
             .replace('api-section-_toc', 'api-section-all') 
        
             .replace('data-id="_toc"', 'data-id="all"');

Trott · 2018-07-07T23:18:08Z

Probably the thing to do is not have the link in all.html?

vsemozhetbyt · 2018-07-07T23:30:08Z

Seems so. We can remove it in the same part. Also, we will need to rebase and update again if https://github.com/nodejs/node/pull/21637/files#diff-faeb3c03dc0d66ce9c5820ab41dc9f58 is landed before this PR.

richardlau · 2018-07-08T02:25:42Z

Is there a danger of encouraging collaborators to commit directly to the master branch instead of raising a PR? I think the edit link also creates branches in the main repo instead of your fork (at least if you have a commit bit)?

silverwind · 2018-07-08T07:47:14Z

tools/doc/html.js

@@ -377,3 +379,8 @@ function altDocs(filename, docCreated) {
    </li>
  ` : '';
 }
+
+function editOnGitHub(filename) {
+  const svg = '<svg style="height: 17px; vertical-align: middle; margin: -2px 5px 0 0;" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M12 0C5.374 0 0 5.373 0 12c0 5.302 3.438 9.8 8.207 11.387.6.11.793-.26.793-.577v-2.234c-3.338.726-4.033-1.416-4.033-1.416-.546-1.387-1.333-1.756-1.333-1.756-1.09-.745.083-.73.083-.73 1.205.085 1.84 1.238 1.84 1.238 1.07 1.834 2.806 1.304 3.49.997.108-.776.42-1.306.763-1.605-2.665-.305-5.467-1.334-5.467-5.93 0-1.312.47-2.382 1.236-3.222-.125-.303-.536-1.524.116-3.176 0 0 1.008-.322 3.3 1.23A11.51 11.51 0 0 1 12 5.803c1.02.005 2.047.138 3.006.404 2.29-1.552 3.297-1.23 3.297-1.23.653 1.653.242 2.874.118 3.176.77.84 1.236 1.91 1.236 3.22 0 4.61-2.807 5.625-5.48 5.922.43.372.824 1.102.824 2.222v3.293c0 .32.192.694.8.576C20.567 21.796 24 17.3 24 12c0-6.627-5.373-12-12-12z"></path></svg>';


Slightly optimized version with xmlns removed because it's inline and .1px viewbox added because of a cropping issue in the original logo:

<svg height="16" width="16" viewBox="0 0 16.1 16.1"><path d="M8 0a8 8 0 0 0-2.5 15.6c.4 0 .5-.2.5-.4v-1.5c-2 .4-2.5-.5-2.7-1 0-.1-.5-.9-.8-1-.3-.2-.7-.6 0-.6.6 0 1 .6 1.2.8.7 1.2 1.9 1 2.4.7 0-.5.2-.9.5-1-1.8-.3-3.7-1-3.7-4 0-.9.3-1.6.8-2.2 0-.2-.3-1 .1-2 0 0 .7-.3 2.2.7a7.4 7.4 0 0 1 4 0c1.5-1 2.2-.8 2.2-.8.5 1.1.2 2 .1 2.1.5.6.8 1.3.8 2.2 0 3-1.9 3.7-3.6 4 .3.2.5.7.5 1.4v2.2c0 .2.1.5.5.4A8 8 0 0 0 16 8a8 8 0 0 0-8-8z"/></svg>

I'm fine with keeping the SVG in here, but the CSS should be in the css file.

sagirk

LGTM! 🎉

I wanted to suggest alt="GitHub Icon" for the SVG, but in retrospect I think it will just add more noise. The Edit on GitHub link text is already pretty sufficient.

silverwind · 2018-07-11T08:32:23Z

Not a fan of loading SVG as <img> as it forbids styling. Can you load the content with a fs.readFile and then paste it inline? In that case, you can omit the xmlns attribute as it is optional for inline SVG.

Also, I suggest applying fill: currentColor to it so it matches the text color like this:

silverwind

See above.

Proof of concept for an "Edit on GitHub" link, inspired by the Serverless docs. One issue is that the link is to the version of the docs on the master branch even if the person was reading a different version of the doc. I don't consider that a big problem, although we can always remove the link if it turns out to be a big problem. I don't think there is a good solution. PRs need to be opened against the master branch generally. Having a bunch of PRs against staging branches is probably not what we want. If there's an update to one version of the doc, there will usually be an update to other versions.

Trott · 2018-07-13T03:13:04Z

The only issue is the all.html

@vsemozhetbyt Fixed. The link is now omitted in all.html.

Trott · 2018-07-13T03:16:35Z

Is there a danger of encouraging collaborators to commit directly to the master branch instead of raising a PR?

@richardlau It's a possibility. I'm inclined to give our Collaborators the benefit of the doubt on this until we have a problem, but that's my opinion only and this is certainly something reasonable people can disagree on. Is this a blocking problem to you (@richardlau) or any of the Collaborators that gave the comment a 👍 (@TimothyGu, @maclover7)?

richardlau · 2018-07-13T03:30:09Z

Not blocking for me -- Just wanted to make sure we're aware of potential downsides.

Trott · 2018-07-13T03:34:39Z

@silverwind I've moved the SVG inline again. PTAL. Thanks.

Trott · 2018-07-13T03:35:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15841/

Proof of concept for an "Edit on GitHub" link, inspired by the Serverless docs. One issue is that the link is to the version of the docs on the master branch even if the person was reading a different version of the doc. I don't consider that a big problem, although we can always remove the link if it turns out to be a big problem. I don't think there is a good solution. PRs need to be opened against the master branch generally. Having a bunch of PRs against staging branches is probably not what we want. If there's an update to one version of the doc, there will usually be an update to other versions. PR-URL: nodejs#21703 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Trott · 2018-07-14T03:37:45Z

Landed in 61cd101

Proof of concept for an "Edit on GitHub" link, inspired by the Serverless docs. One issue is that the link is to the version of the docs on the master branch even if the person was reading a different version of the doc. I don't consider that a big problem, although we can always remove the link if it turns out to be a big problem. I don't think there is a good solution. PRs need to be opened against the master branch generally. Having a bunch of PRs against staging branches is probably not what we want. If there's an update to one version of the doc, there will usually be an update to other versions. PR-URL: #21703 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Yuta Hiroto <hello@hiroppy.me> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Roman Reiss <me@silverwind.io>

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jul 7, 2018

apapirovski approved these changes Jul 7, 2018

View reviewed changes

silverwind reviewed Jul 8, 2018

View reviewed changes

fhinkel approved these changes Jul 8, 2018

View reviewed changes

hiroppy approved these changes Jul 8, 2018

View reviewed changes

JungMinu approved these changes Jul 9, 2018

View reviewed changes

PeterDaveHello approved these changes Jul 9, 2018

View reviewed changes

Trott force-pushed the edit-on-github branch from be52412 to 821e484 Compare July 10, 2018 22:47

sagirk approved these changes Jul 11, 2018

View reviewed changes

silverwind suggested changes Jul 11, 2018

View reviewed changes

Trott added 4 commits July 12, 2018 19:55

squash! update svg per silverwind

9d6f38d

squash! move css and svg to own files

3620da2

squash! do not include edit-on-github link in all.html

d0df43f

Trott force-pushed the edit-on-github branch from e4c2b1c to d0df43f Compare July 13, 2018 03:13

squash! move svg inline again

c69cae7

silverwind approved these changes Jul 13, 2018

View reviewed changes

ckotzbauer approved these changes Jul 13, 2018

View reviewed changes

Trott closed this Jul 14, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

richardlau mentioned this pull request Jul 19, 2018

doc - Document MODULE_NOT_FOUND error code #21890

Closed

refack mentioned this pull request Oct 31, 2018

docs: When using the "Edit on GitHub" button references to the guides are not obvious #24003

Closed

Trott deleted the edit-on-github branch January 13, 2022 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add "Edit on GitHub" link #21703

doc: add "Edit on GitHub" link #21703

Trott commented Jul 7, 2018

nodejs-github-bot commented Jul 7, 2018

Trott commented Jul 7, 2018

Trott commented Jul 7, 2018

Trott commented Jul 7, 2018

vsemozhetbyt commented Jul 7, 2018 •

edited

Loading

vsemozhetbyt commented Jul 7, 2018

Trott commented Jul 7, 2018

vsemozhetbyt commented Jul 7, 2018

richardlau commented Jul 8, 2018

silverwind Jul 8, 2018 •

edited

Loading

sagirk left a comment

silverwind commented Jul 11, 2018 •

edited

Loading

silverwind left a comment

Trott commented Jul 13, 2018 •

edited

Loading

Trott commented Jul 13, 2018

richardlau commented Jul 13, 2018

Trott commented Jul 13, 2018

Trott commented Jul 13, 2018

Trott commented Jul 14, 2018

doc: add "Edit on GitHub" link #21703

doc: add "Edit on GitHub" link #21703

Conversation

Trott commented Jul 7, 2018

Checklist

nodejs-github-bot commented Jul 7, 2018

Trott commented Jul 7, 2018

Trott commented Jul 7, 2018

Trott commented Jul 7, 2018

vsemozhetbyt commented Jul 7, 2018 • edited Loading

vsemozhetbyt commented Jul 7, 2018

Trott commented Jul 7, 2018

vsemozhetbyt commented Jul 7, 2018

richardlau commented Jul 8, 2018

silverwind Jul 8, 2018 • edited Loading

Choose a reason for hiding this comment

sagirk left a comment

Choose a reason for hiding this comment

silverwind commented Jul 11, 2018 • edited Loading

silverwind left a comment

Choose a reason for hiding this comment

Trott commented Jul 13, 2018 • edited Loading

Trott commented Jul 13, 2018

richardlau commented Jul 13, 2018

Trott commented Jul 13, 2018

Trott commented Jul 13, 2018

Trott commented Jul 14, 2018

vsemozhetbyt commented Jul 7, 2018 •

edited

Loading

silverwind Jul 8, 2018 •

edited

Loading

silverwind commented Jul 11, 2018 •

edited

Loading

Trott commented Jul 13, 2018 •

edited

Loading