Skip to content

improve: add "EditOnGitHub" on every necessary page #3971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 16, 2021
Merged

improve: add "EditOnGitHub" on every necessary page #3971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a1e83c
moveToFooter
Jul 15, 2021
c440b44
Merge branch 'master' into fixEditOnGit
Jul 15, 2021
b1749ab
Merge branch 'master' into fixEditOnGit
Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion layouts/download-current.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<div class="download-header">
<h1>{{downloads.headline}}</h1>
</div>

{{{contents}}}
{{> primary-download-matrix version=project.latestVersions.current versionTypeCurrent="true"}}
{{> secondary-download-matrix version=project.latestVersions.current versionTypeCurrent="true"}}

Expand Down
2 changes: 1 addition & 1 deletion layouts/download.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<div class="download-header">
<h1>{{downloads.headline}}</h1>
</div>

{{{contents}}}
{{> primary-download-matrix version=project.latestVersions.lts versionTypeLts="true"}}
{{> secondary-download-matrix version=project.latestVersions.lts versionTypeLts="true"}}

Expand Down
1 change: 1 addition & 0 deletions layouts/partials/footer.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
<img src="/static/images/openjs_foundation-logo.svg" alt="OpenJS Foundation" width="120" height="38">
</a>
<ul class="list-divider-pipe issue-link">
<li><a id="editOnGitHubLink" href="#">{{site.editOnGithub}}</a></li>
<li><a href="https://github.com/nodejs/node/issues">{{site.reportNodeIssue}}</a></li>
<li><a href="https://github.com/nodejs/nodejs.org/issues">{{site.reportWebsiteIssue}}</a></li>
<li><a href="https://github.com/nodejs/help/issues">{{site.getHelpIssue}}</a></li>
Expand Down
1 change: 1 addition & 0 deletions locale/en/site.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{
"title": "Node.js",
"editOnGithub":"Edit On GitHub",
"author": "Node.js",
"url": "https://nodejs.org/en/",
"locale": "en",
Expand Down
21 changes: 11 additions & 10 deletions scripts/plugins/githubLinks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
'use strict'

const sep = require('path').sep
// add suffix (".html" or sep for windows test) to each part of regex
// to ignore possible occurrences in titles (e.g. blog posts)
const isEditable = `(security|index).html|(about|download|docs|foundation|get-involved|knowledge)\\${sep}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't see why this is needed. These are permalinks, so they will always use / regardless of the OS.

Copy link
Contributor Author

@SEWeiTung SEWeiTung Jul 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, for Linux and other systems (exclude windows), the original codes work perfectly. However as you see, if for windows (I'm now working on Win, and because it uses '' instead of '/' as the path sep, so I cannot see the result right or not according to '/' in the gitHubLink.js (because nothing would be found and replaced).

So in order to be compatable with all the platforms, sep is a better choice IMO :)

Or maybe any good other ideas?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand. Are you viewing it with a file: URL or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Trott and @XhmikosR : No. what I mean is: if we do tests on Windows, because the "files" in "scripts\plugins\githubLinks.js" will have the spe = "", however the original filtering condition is something like "downloads/", this means for windows platform, we cannot get the result (we cannot see "Edit on GitHub" on the locally running test page).

Copy link
Contributor

Choose a reason for hiding this comment

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

I explained it to you... these are links. They will always use /. This is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@XhmikosR : I see you mean for the output files, yes, they will always use "/", but on windows platform in gitHhubLinks.js, you'll never get any files to be coped with, because windows is using "", this means if you using "npm run serve", you will never see the result of "Edit on GitHub" on the page, and you don't know what you did is right or not.

So considering those who are using windows, I have to use spe instead of hard code here. You can have a try on windows, if you hard coded, then you know why I changed to this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, these are links so they will always use / regardless of OS. That is why it is working all this time just fine.

const isEditableReg = new RegExp(isEditable)

// This middleware adds "Edit on GitHub" links to every editable page
function githubLinks (options) {
return (files, m, next) => {
// add suffix (".html" or "/") to each part of regex
// to ignore possible occurrences in titles (e.g. blog posts)
const isEditable = /security\.html|about\/|docs\/|foundation\/|get-involved\/|knowledge\//

Object.keys(files).forEach((path) => {
if (!isEditable.test(path)) {
if (!isEditableReg.test(path)) {
return
}

const file = files[path]
const url = `https://github.com/nodejs/nodejs.org/edit/master/locale/${options.locale}/${path.replace('.html', '.md')}`
const editText = options.site.editOnGithub || 'Edit on GitHub'
path = path.replace('.html', '.md').replace(/\\/g, '/')
const url = `https://github.com/nodejs/nodejs.org/edit/master/locale/${options.locale}/${path}`

const contents = file.contents.toString().replace(/<h1(.*?)>(.*?)<\/h1>/, (match, $1, $2) => {
return `<a class="edit-link" href="${url}">${editText}</a> <h1${$1}>${$2}</h1>`
})
const contents = file.contents.toString() +
` <input type = "hidden" id = "editOnGitHubUrl" value="${url}"/> `

file.contents = Buffer.from(contents)
})
Expand Down
14 changes: 14 additions & 0 deletions static/js/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,3 +246,17 @@
winText.textContent = winText.textContent.replace(/x(86|64)/, arch)
}
})()

;(function () {
// This function is used to replace the anchor
// link of Edit on GitHub

var editOnGitHubElement = document.getElementById('editOnGitHubLink')
var editOnGitHubUrlElement = document.getElementById('editOnGitHubUrl')

if (editOnGitHubUrlElement) {
editOnGitHubElement.setAttribute('href', editOnGitHubUrlElement.value)
} else {
editOnGitHubElement.parentNode.parentNode.removeChild(editOnGitHubElement.parentNode)
}
})()