improve: add "EditOnGitHub" on every necessary page #3971
Conversation
|
The separator isn't even needed because those are permalinks so they use forward slashes. As for the change itself, I can't really comment on it. I like simple things and this is something I personally don't like. |
|
Now after my deep thoughts in both UI and codes, I've changed by moving a GitHub icon onto the header, just like what others' website by GitHub. It looks more beautiful and practical:
For diff language, it has the default tooltip: Here's the missing download part: Codes look simple but useful, easy to understand ;) |
|
Ping @nodejs/website : Any other ideas on it? I'll leave it for some days for your suggestions. |
|
Maybe this could go in a footer or sidebar. |
Thanks for your suggestion, many website pages on GitHub has a link on its head, and to be more clear, GitHub's icon is better:) Take React as an example:
Can you point out where to put by circuling in red with a screenshot here ;) |
| 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}` |
There was a problem hiding this comment.
I still don't see why this is needed. These are permalinks, so they will always use / regardless of the OS.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm not sure I understand. Are you viewing it with a file: URL or something like that?
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
I explained it to you... these are links. They will always use /. This is not needed.
There was a problem hiding this comment.
@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 :)
There was a problem hiding this comment.
No, these are links so they will always use / regardless of OS. That is why it is working all this time just fine.
|
I guess there isn't a sidebar on most pages, but the header and footer could work. React is a good example, and we already have a Google Translate button in the header anyway. |
Yes, that's also what I think now by just putting it beside the Mul-language icon :) |
|
Nothing against GitHub, but I'm not excited about prominently featuring a company's logo on all webpages like that. I just removed the GitHub mark from docs a few days ago. nodejs/node#39251 |
Sorry I didn't notice it....I'll change it to "Edit on GitHub" as a link and put it there. |
default.mp4 |
|
Sounds good to me. Anyone looking at this more closely want to review it? |
|
I don't think that button should be in the header. I'd put it into into the footer instead, besides |
Since @silverwind and @nickmccurdy have ideas to put it in the footer.... I've changed to this following:
|
|
@MaledongGit I still don't get why you use path.sep. We are 100% sure there will not be a Secondly, why do we even need the new JS at all? We have already access to the page path in metalsmith, This shouldn't have landed IMHO. |
Here's the 1st version (Deprecated now, just keep for history):