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

feat: add i18nPaths for selective i18n routing #2131

Closed
wants to merge 26 commits into from

Conversation

anshgoyalevil
Copy link
Member

@anshgoyalevil anshgoyalevil commented Sep 9, 2023

Description

  • This PR adds the i18nPaths for selective routing. Since there are multiple pages on the website that still need translation, the selective LinkComponent behaves like next/link component and selects the localized paths from i18nPaths file to redirect the user according to their localization.
  • This is needed in case there exist translations for a page in one language, but not in the other language. The i18nPaths component would intelligently select the localized path and redirect according to it.

Related issue(s)
part of: #2039

How to test changes?

@netlify
Copy link

netlify bot commented Sep 9, 2023

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 6a25760
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65bd27d4e02ae40008ea5251
😎 Deploy Preview https://deploy-preview-2131--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link

github-actions bot commented Sep 9, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 22
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🔴 PWA 30

Lighthouse ran on https://deploy-preview-2131--asyncapi-website.netlify.app/

Signed-off-by: Ansh Goyal <[email protected]>
@anshgoyalevil
Copy link
Member Author

//cc @magicmatatjahu 🚀

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Two suggestions, but second one is crucial to add :) Amazing work!

components/link.js Outdated Show resolved Hide resolved
components/link.js Outdated Show resolved Hide resolved
@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Nov 21, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 34
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-2131--asyncapi-website.netlify.app/

Comment on lines +26 to +32
if ((props.href && i18nPaths[language] && !i18nPaths[language].includes(href)) || href.startsWith("http")) {
return (
<Link {...props} href={href} passHref>
{children}
</Link>
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Which usecase is handled in this if condition? Kindly specify that in comment.

Copy link
Member Author

@anshgoyalevil anshgoyalevil Feb 2, 2024

Choose a reason for hiding this comment

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

It would handle the cases where a webpage is not yet translated or there is an external website's href

Comment on lines +1 to +8
const i18nPaths = {
en: [
"/tools/cli"
],
de: [
"/tools/cli"
]
};
Copy link
Member

Choose a reason for hiding this comment

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

What is the use of these i18nPaths?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would work as a record for the translated links. Since we are using static site generation, we needed a mechanism to predefine the translated webpages which could be used by the i18n LinkComponent

@@ -1,3 +1,4 @@
import LinkComponent from '../link';
import Paragraph from '../typography/Paragraph';
import Label from './Label'
import Link from 'next/link'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import Link from 'next/link'

Copy link
Member

@akshatnema akshatnema left a comment

Choose a reason for hiding this comment

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

We have these same set of changes in #2184. Let's merge these merges with that PR only and enable all the options in one go 🚀 .

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