-
-
Notifications
You must be signed in to change notification settings - Fork 632
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 translation hooks and packages #1980
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-1980--asyncapi-website.netlify.app/ |
German 👀 |
@derberg I was testing the translation thing, and sticked to the German Language. Would add Spanish Language (or maybe Polish) in the upcoming PRs. |
@anshgoyalevil @derberg Yeah, it was my fault. I suggested German only for testing (we don't need the proper translation tbh, we can even use lorem ipsum on German texts). However if we wanna go with "production" translations we can go with Spanish or Polish from beginning. Sorry for that misunderstood! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! 🚀 I see some "problems" but overall it's great start. We need to remember (before merge) to disable the language switcher and also add the proper blockers in the crawler bots config to disable crawling (now) the translations. We will enable that in last PR :)
@magicmatatjahu Done with the changes. Thanks for the inputs 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are migrating the file, can you please do the refactoring of this codebase in terms of formatting and spacing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
lib/locales.js
Outdated
const locales = {}; | ||
languages.map((language) => { | ||
locales[language] = {}; | ||
|
||
namespaces.map((namespace) => { | ||
locales[language][namespace] = require("./../locales/" + | ||
language + | ||
"/" + | ||
namespace + | ||
".json"); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly wrap this in a function and then return the required response from the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this is not my implementation. It is suggested by the plugin author since were in need to use the static site built due to Netlify restrictions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please share the source you are following to create this file?
IMHO, even if we have to implement such logic, I don't think it will make any difference while implementing it inside functions. Reason why I'm asking for making it as a function is that we should work on code quality as well, instead of just writing it as plain implementation of js. Also, it will be easier to add unit tests (later on) for this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I totally agree with your points.
I am following this package. Their one of the examples is what I tried imitating in our project.
The reason to use this package is that previously I was using the next-i18next package to handle the translation-related logic, and everything was going smoothly inside the local dev. environment.
As I pushed that code, it became useless because Netlify cannot process translations on the server, and need pre-generated translations.
Another option was to use a Node.JS environment to host the website, but that doesn't seem viable.
Co-authored-by: Akshat Nema <[email protected]>
Thanks for the inputs @akshatnema 🚀 This PR is ready to be merged. The scope of this PR is limited to:
|
@anshgoyalevil Kindly resolve the comments only if either the discussion made using the comment satisfies the reviewer or you made changes requested by the reviewer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok for me :) Please only change the label for Github button and we can merge :)
Co-authored-by: Maciej Urbańczyk <[email protected]>
Thanks @magicmatatjahu 😄. Changed the GitHub Button 🚀 |
@anshgoyalevil It still has Please fix that :) It has to be: |
@magicmatatjahu Sorry for that. Missed that. Fixed now |
@akshatnema Hi! Could you check PR? |
/rtm |
Description
en
Related issue(s)
fixes #1978