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

Assorted link fixes. #2563

Merged
merged 1 commit into from
Sep 16, 2019
Merged

Assorted link fixes. #2563

merged 1 commit into from
Sep 16, 2019

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Sep 9, 2019

There's probably a lot more...

But in order to speed things up, we need to make any doc links relative to the server root when possible, and use the absolute URLs only for files that are not generated in this repo.

For example, https://nodejs.org/en/blog/ should become /en/blog/. Being that this change will be a big one, I first need confirmation before moving with it.

I've managed to scan for internal links for the English version in my linkinator branch. Travis CI build: https://travis-ci.org/XhmikosR/nodejs.org/builds/582621043
There's one failure only due to #2422, which I'm unsure how to solve.

That being said, if we agree on the relative vs absolute links, I'll make a PR with linkinator so that others can chime in and help.

@XhmikosR
Copy link
Contributor Author

Can we move forward with this?

@Trott
Copy link
Member

Trott commented Sep 13, 2019

It's not clear to me that https://nodejs.org/en/blog/ is better than https://nodejs.org/en/blog. I know old-school HTML thinking was that you should always make it explicit that it's a directory when it's not a file. That avoids an extra redirect for the end user. But this isn't necessarily a filesystem--I mean, it is in our case and there is a redirect--but I don't know about all of these? And supposedly /blog will give better SEO than /blog/?

Anyway, not opposing this or anything. Just explaining why I haven't offered an opinion. I'm not sufficiently informed about current best practices etc.

Let's ping @nodejs/website and see if people can approve or explain why not.

Feel free to open a smaller PR with just a few of these if there are some that are obvious slam-dunk must-change links so that they aren't held up by other links that perhaps need discussion.

@XhmikosR
Copy link
Contributor Author

The missing trailing slash is a useless redirect.

@XhmikosR
Copy link
Contributor Author

I fixed more links. There's probably even more but I want to do it in batches and also we need #2565 eventually.

@XhmikosR
Copy link
Contributor Author

OK, this is done. I don't want to push more stuff to keep it easier to review.

The only thing that I'm not sure is what's the policy to linking to translated files. I mean, I know in production we generate all the missing files from translations, but linking to them isn't consistent AFAICT.

So, for the time being, I only link to translated pages.

@Trott
Copy link
Member

Trott commented Sep 15, 2019

Lots of files changed, but fewer than 300 lines. Anyone up for reviewing this? @nodejs/website (I'd hate to ask @XhmikosR to split this into multiple PRs to make reviewing easier but if that's what we gotta do to get some folks to review it sufficiently, then so be it.)

@@ -43,7 +43,6 @@ La [secció d'ES6](/en/docs/es6/) descriu l'arbre dels grups de les característ
característiques són activades per defecte en Node.js, juntament amb enllaços explicatius. També mostra com trobar
quina versió de V8 fa servir una versió particular de Node.js.

## Preguntes freqüents
## Guides
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you changed translation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there's no /faq link and I don't speak ca.

locale/ko/about/resources.md Outdated Show resolved Hide resolved
* use relative to root URLs for files generated in this repo
* use the absolute URL for all other URLs
* fix 404s
@SEWeiTung SEWeiTung merged commit 5f41c8d into nodejs:master Sep 16, 2019
@XhmikosR XhmikosR deleted the master-xmr-link-fixes branch September 16, 2019 08:04
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.

5 participants