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

Add react-i18next for String Localization #51

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

techmannih
Copy link

@techmannih techmannih commented Dec 14, 2024

close #39
@quest-bot loot #39

@gemanor I want to know if I am going in the right direction, and what you think about the implementation. Your feedback and advice give me a lot of confidence. After your confirmation, I will proceed with implementing the other commands

@gemanor
Copy link
Collaborator

gemanor commented Dec 15, 2024

Hey, looks like a great start. Soe comments:

  1. Please merge from main. There's a large change in Login flow
  2. I think we don't need the React library. It seems like you can just initialize the i18next and use it (we might not need the cache of the react-i18next / next-i18next. Please let me know if I'm missing something.
  3. Please use the translations in the .json file, import, and import it.

TIA

@techmannih
Copy link
Author

techmannih commented Dec 17, 2024

@gemanor yes, this branch is up to date with main branch please review this PR

@gemanor
Copy link
Collaborator

gemanor commented Dec 29, 2024

Hey @techmannih, your PR is missing many strings in the components / lib folder. It also has many styling issues (run npm run test / npm run lint to find them).
Please address them in the next 48 hours, as we want to close this PR and already have some others waiting.

@techmannih
Copy link
Author

@gemanor please review now

@gemanor
Copy link
Collaborator

gemanor commented Dec 30, 2024

@techmannih seems like the lint/prettier is still failing (see the CI tests..)

@gemanor
Copy link
Collaborator

gemanor commented Dec 30, 2024

Hey @techmannih , all the tests are passing now on main
Please merge from main and ensure all the tests is passing

@techmannih
Copy link
Author

techmannih commented Dec 30, 2024

@gemanor I am able to see any command in components / lib. can you help me to show

@techmannih
Copy link
Author

techmannih commented Dec 30, 2024

@gemanor now all test pass, please check
image

@gemanor
Copy link
Collaborator

gemanor commented Dec 30, 2024

Hey @techmannih, there are many strings in the components/hooks folders that are still not included in the i18n support. Please make sure you include them too..

@gemanor
Copy link
Collaborator

gemanor commented Dec 30, 2024

Tests are good now indeed :)

@techmannih
Copy link
Author

techmannih commented Dec 30, 2024

@gemanor these are not command string there are only error and text string, I think we are fixing command string, please confirm, if I am correct

Hey @techmannih, there are many strings in the components/hooks folders that are still not included in the i18n support. Please make sure you include them too..

@gemanor
Copy link
Collaborator

gemanor commented Dec 30, 2024

It should be on all strings. Actually, everything in the components is something that renders as part of the commands. The command folder is only the high-level component of the commands

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.

Add react-i18next for String Localization
2 participants