-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Qol][Refactor] i18n lazy-loading #4327
Conversation
Review Helperto mark all $$("#files .js-reviewed-toggle > input[id*='.json']").forEach((el) => el.click()); Your browser will probably lagg for 1-3 minutes as it sends quite a lot of server-requests at the same time then Or use this link: It uses the filters to exclude all |
we don't need to test if i18next is working. This is the job of i18next itself
instead of the english translation
with lazy-load
now using "mysteryEncounters/..."
the encounter was checking if the modifier name includes `Berry` which is only true for english. Instead it has to check if the modifier is an instance of BerryModifier
the new i18n pattern requires a different namespacing which has been adopted
there was an issue with circular dependencies
Found a missing change from pokerogue/src/data/mystery-encounters/encounters/the-expert-pokemon-breeder-encounter.ts Line 543 in c287584
Will try and double check all ME files |
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.
Found 2 keys not properly updated for MEs:
await showEncounterText(scene, `${namespace}.option.1.bad`); |
should be
await showEncounterText(scene,
${namespace}:option.1.bad);
and
pokerogue/src/data/mystery-encounters/encounters/the-expert-pokemon-breeder-encounter.ts
Line 543 in c287584
text: `${namespace}.outro_failed`, |
should be text:
${namespace}:outro_failed,
and remove `nonExplicitSupportedLngs`
Caution
Make sure to consult the @pagefaultgames/translation-team & @pagefaultgames/translation-leads before merging as all locale files moved from
/src/locales
into/public/locales
!!!Tip
there's only
~7
relevant files, the rest are all file moves/etc- NightKev
What are the changes the user will see?
Only the languages being used will be loaded thus less memory usage from the i18n side
Why am I making these changes?
The memory shouldn't be bloated with all languages at all time. IT should only hold the languages currently being used which at max is 2 (en as a fallback for missing keys).
What are the changes from a developer perspective?
/src/locales
into/public/locales
as they will be loaded async (via network request).camelCaseToKebabCase
function to utils/src/plugin/i18n.ts
file structurestartGame()
and lazy-load the scenes to prevent anyi18n.t()
usages before it is initializedinitI18n()
fromLoadingScene
config.ts
files inside thelocales/
locales
tests. Reason: We don't need to test if i18next is working properly. They have their own test-coverage to ensure thati18next
is now mocked for tests. This was overdue and now necessary due to the backend moduleHow to test the changes?
locales/**
)Checklist
beta
as my base branch[ ] Have I considered writing automated tests for the issue?[ ] If I have text, did I make it translatable and add a key in the English locale file(s)?npm run test
)[ ] Are the changes visual?[ ] Have I provided screenshots/videos of the changes?