Skip to content
This repository has been archived by the owner on Mar 14, 2024. It is now read-only.

Make translation link work with explicit language in path #3670

Merged
merged 8 commits into from Aug 19, 2020

Conversation

devnook
Copy link
Contributor

@devnook devnook commented Aug 10, 2020

Currently. we use only canonical urls (/foo) internally in web.dev. The only exception was where from a /lang/ page we link to canonical one (e.g. from /pl/foo to /en/foo, to enforce language version of the target despite user preferences). This Pr forces this link to open in a new page, to trigger a server-side redirect.

At the same time, I removed a server-side redirection for partials (from /en/foo/index.json to /foo/index.json) as it was unnecessary.

@devnook devnook requested a review from a team as a code owner August 10, 2020 13:58
@googlebot googlebot added the cla: yes Contributor has signed the CLA label Aug 10, 2020
@netlify
Copy link

netlify bot commented Aug 10, 2020

Deploy preview for web-dev-staging ready!

Built with commit 488aa1d

https://deploy-preview-3670--web-dev-staging.netlify.app

@@ -8,7 +8,7 @@
API.
{% elif translation === 'manual' %}
This article was translated from
[the original post](/en{{ page.data.canonicalUrl }}).
<a href="/en{{ canonicalUrl }}" target="_blank">the original post</a>.
Copy link
Contributor

Choose a reason for hiding this comment

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

@samthor could we add a data attribute to links to signal to the router to ignore them? My concern with relying on target="_blank" here is that someone might one day remove it and not realize it has implications with the server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment made me realise that we do skip absolute urls in the router, so I changed an internal /en/ link to an absolute one.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah clever!

Would you mind also adding a comment to njk to explain why youre doing the full url

@robdodson
Copy link
Contributor

@devnook this looks good to merge. Just remember to add a comment to the html explaining why it's using a full path.

@devnook devnook merged commit 3157aec into master Aug 19, 2020
@devnook devnook deleted the devnook-links branch August 19, 2020 10:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Contributor has signed the CLA eng - i18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants