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: enable i18n support for AsyncAPI website #2184

Merged
merged 43 commits into from
Feb 11, 2024

Conversation

anshgoyalevil
Copy link
Member

Description

This PR aims to finally enable the i18n for all users.

How to test?
Use the language picker at the top right corner to play with EN and DE languages.

Related issue(s)

@netlify
Copy link

netlify bot commented Sep 28, 2023

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit e6c95e0
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65c87f4d2b27140008a84bc3
😎 Deploy Preview https://deploy-preview-2184--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.

@anshgoyalevil anshgoyalevil changed the title Enable i18n support for AsyncAPI Enable i18n support for AsyncAPI website Sep 28, 2023
@anshgoyalevil
Copy link
Member Author

// @magicmatatjahu 🚀

@anshgoyalevil anshgoyalevil changed the title Enable i18n support for AsyncAPI website feat: Enable i18n support for AsyncAPI website Sep 28, 2023
@anshgoyalevil anshgoyalevil changed the title feat: Enable i18n support for AsyncAPI website feat: enable i18n support for AsyncAPI website Sep 28, 2023
@github-actions
Copy link

github-actions bot commented Sep 28, 2023

⚡️ Lighthouse report for the changes in this PR:

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

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

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.

Hmm, I tested that in preview and if I change the default one to the de I see the new content but selector isn't changed and also I cannot change back to the en. Also browser is freezed, probably by some long time operation - probably we have in code the infinite recursion (saving to localstorage?). Could you check that?

@anshgoyalevil
Copy link
Member Author

I just checked the code. The value prop of Select component required an update.

About that freeze thing, It is due to the static site rendering. It won't happen when deployed on Netlify, or when viewing the website from the build folder rather than the development preview.

Actually, what happens is:

  • In npm run dev preview, the website's home page is built in EN language.
  • After we change the language from lang picker, the new page is built in German language.
  • This continues as we visit other pages.

In production built, all pages in all locales are pre-built, so this won't happen

@anshgoyalevil
Copy link
Member Author

anshgoyalevil commented Oct 9, 2023

This is recommended to be merged after: #2131

@anshgoyalevil
Copy link
Member Author

anshgoyalevil commented Feb 4, 2024

@akshatnema I have fixed that infinite loop issue, and have added the comments to other functions as well 🚀

Comment on lines 91 to 92
// First param: Passes the language based on the browser's defalt language
// Second param: Prevents the language change from being saved in the local storage
Copy link
Member

Choose a reason for hiding this comment

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

Don't add the function description here. Instead specify, what is the main functionality done by useEffect at initial render.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

// If no unique languages are found, default to ["EN"]
return uniqueLangs.length === 0 ? ["EN"] : uniqueLangs;
}
const uniqueLangs = getUniqueLangs().map((lang) => ({
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
const uniqueLangs = getUniqueLangs().map((lang) => ({
const uniqueLangs = getUniqueLangs().map((lang) => ({

Copy link
Member Author

Choose a reason for hiding this comment

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

done

text: lang,
value: lang
}));

const changeLanguage = async (locale, langPicker) => {
Copy link
Member

Choose a reason for hiding this comment

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

Add jsdocs to specify the functionality and params of this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@akshatnema
Copy link
Member

@anshgoyalevil have you covered the changes of #2181 ?

pages/index.js Outdated
Comment on lines 34 to 37
<Head />
<StickyNavbar>
<NavBar className="max-w-screen-xl block px-4 sm:px-6 lg:px-8 mx-auto" />
</StickyNavbar>
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove Head and Navbar on initial render. They should be shown on when user first visits on the website. The preloader should be part of main section of the website and also it should be centrally placed over the screen.

Comment on lines 107 to 112
/*
useEffect to set the initial language on component mount
Detects the user's preferred language using browserLanguageDetector
and updates the language without affecting the localStorage.
This ensures the components renders with the correct initial language.
*/
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
/*
useEffect to set the initial language on component mount
Detects the user's preferred language using browserLanguageDetector
and updates the language without affecting the localStorage.
This ensures the components renders with the correct initial language.
*/
/**
* useEffect to set the initial language on component mount
* Detects the user's preferred language using browserLanguageDetector
* and updates the language without affecting the localStorage.
* This ensures the components renders with the correct initial language.
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

done 🚀

@anshgoyalevil
Copy link
Member Author

anshgoyalevil commented Feb 6, 2024

@anshgoyalevil have you covered the changes of #2181 ?

No, I think we should keep the separation of concerns.

@akshatnema
Copy link
Member

No, I think we should keep the separation of concerns.

Sorry, I'm talking about #2131 . I see similar changes in both of them.

@anshgoyalevil
Copy link
Member Author

No, I think we should keep the separation of concerns.

Sorry, I'm talking about #2131 . I see similar changes in both of them.

Oh yes that one. Yes, all changes are synced with this PR.

The infinite loop issue again occurred. I don't know why that happens when we have elements like NavBar on the index.js (Since, this error was resolved when we had only loader in index.js). So, this PR is still not ready 🙂

@akshatnema
Copy link
Member

The infinite loop issue again occurred. I don't know why that happens when we have elements like NavBar on the index.js (Since, this error was resolved when we had only loader in index.js). So, this PR is still not ready 🙂

Do let me know once it's fixed.

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

@akshatnema I have fixed that. Please check :)

@anshgoyalevil
Copy link
Member Author

@akshatnema Should we merge this in? 🚀

@akshatnema akshatnema merged commit 7543a56 into asyncapi:master Feb 11, 2024
18 checks passed
@anshgoyalevil anshgoyalevil deleted the enable-i18n branch February 24, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants