Skip to content
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.

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

fix Edit on GitHub #1853

Merged
merged 2 commits into from
Oct 26, 2018
Merged

fix Edit on GitHub #1853

merged 2 commits into from
Oct 26, 2018

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Oct 25, 2018

Now:
screen shot 2018-10-25 at 10 39 11 am

After fixed, Edit on GitHub shows at right side.
screen shot 2018-10-25 at 10 39 18 am

build.js Outdated
const contents = file.contents.toString().replace(/<h1>(.+)<\/h1>/, ($1, $2) => {
return `<a class="edit-link" href="${url}">Edit on GitHub</a> <h1>${$2}</h1>`
const contents = file.contents.toString().replace(/<h1(.*?)>(.*?)<\/h1>/, ($1, $2, $3) => {
return `<a class="edit-link" href="${url}">Edit on GitHub</a> <h1>${$3}</h1>`
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps h1 props should be hold. like this:

return `<a class="edit-link" href="${url}">Edit on GitHub</a> <h1${$2}>${$3}</h1>`

build.js Outdated
@@ -185,8 +185,8 @@ function githubLinks (options) {
const file = files[path]
const url = `https://github.com/nodejs/nodejs.org/edit/master/locale/${options.locale}/${path.replace('.html', '.md')}`

const contents = file.contents.toString().replace(/<h1>(.+)<\/h1>/, ($1, $2) => {
return `<a class="edit-link" href="${url}">Edit on GitHub</a> <h1>${$2}</h1>`
const contents = file.contents.toString().replace(/<h1(.*?)>(.*?)<\/h1>/, ($1, $2, $3) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The callback signature was a bit misleading, should be (match, $1, $2).
You can also get rid of the first capture group (since it's unused). /<h1[^>]+>(.*?)<\/h1>/

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your help. So I don't need to change it anymore right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we're good now. ;)

@fhemberger fhemberger merged commit 801a51c into nodejs:master Oct 26, 2018
@ZYSzys ZYSzys deleted the edit-on-github branch October 26, 2018 14:16
@refack
Copy link
Contributor

refack commented Nov 6, 2018

@ZYSzys, when you open a PR there's a checkbox named "Allow edits from maintainers."
image
This gives other mainteners write permission to branch in the PR. You can use it with the GitHub pancil button
image
Or from you computer using a full URL with git push, e.g.:

git push https://github.com/zys-contribs/nodejs.org edit-on-github

@ZYSzys
Copy link
Member Author

ZYSzys commented Nov 7, 2018

@refack Understand now, thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants