-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/covid19india/covid19india-react/5zy1f8php |
fc07931
to
fb4bcd9
Compare
@harshzalavadiya Hey, so this feature is still in staging and we're looking forward to merge soon! I'm not familiar with with i18n library so I was stumped to figure out how to fix it myself, would appreciate it if you could fix that! Also we need to support these languages on the first push:
I think there are json files for languages not mentioned above, feel free to keep them also! More the languages, the better! You don't have to work on all the translations, it's under stable how much an arduous task that is! Let me know what you think :) Really awesome work btw! Heads up: This is a separate branch now, so your PRs will have to build upon this! |
sure @jeremyphilemon I'll be happy to fix |
src/App.js
Outdated
const history = require('history').createBrowserHistory; | ||
|
||
function App() { | ||
const {t} = useTranslation(); |
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.
const { translate } = useTransaltion();
translate
would be more meaningful than just t
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.
Hook is from react-i18next and we can't change it directly https://react.i18next.com/latest/usetranslation-hook
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 translate
is a must, then this might help
const { t: translate } = useTransaltion();
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.
IMHO, @KrishnaPravin, I know aliasing is possible but doing it everywhere, or wrapping it in another hook isn't a clean approach.
they are naming function as 't' because it's less distractive provides better DX, ctrl clicking on t goes to useTranslate that anyway says that this is a translate function plus most react translation libraries do it same way.
https://www.npmjs.com/package/react-translate
https://react.i18next.com/latest/usetranslation-hook
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.
understood👍
2156437
to
3408b67
Compare
3408b67
to
7f75ad6
Compare
@jeremyphilemon I guess this branch is far outdated to be merged with master, Humble advise: next time if localisation happens it should be merged with master in day or two since it affects almost all files, don't wait for all translations to be perfect Just merge it once and then keep on improving it. Otherwise merge conflicts will keep on happening and it will end up in similar situation Anyways it was a nice learning experience for me keep up the good work |
7f75ad6
to
0b9d18a
Compare
Closing in favor of #1862 :) |
Feature added by: @harshzalavadiya
Co-authored-by: Jeremy jeremyphilemon@outlook.com