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

[Refactor][Localization] Better handling for StatusEffect.NONE with i18n #4405

Merged

Conversation

flx-sta
Copy link
Collaborator

@flx-sta flx-sta commented Sep 24, 2024

Found this inconvenience for translators with @CodeTappert

What are the changes the user will see?

None

Why am I making these changes?

Currently there is the necessity in translation of a few empty keys.
Quite inconvenient and useless. Reason for that is to handle the theoretical situation of statusEffect:none.<obtain | heal etc.> Which realistically never occurs afaik

What are the changes from a developer perspective?

  1. All getStatusEffect<...>Text() methods now check for StatusEffect.NONE to simply return an empty-string ""
  2. Removed all those (now) obsolete keys from status-effect.json file/s
  3. Fixed the tests (Which ironically will be removed in i18n-lazy-load anyway)

How to test the changes?

You can't really. Just set a status and remove it again and see if something odd happens.

Example Overrides

const overrides = {
  OPP_MOVESET_OVERRIDE: Moves.THUNDER_WAVE,
  MOVESET_OVERRIDE: [Moves.REFRESH]
} satisfies Partial<InstanceType<typeof DefaultOverrides>>;

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • 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)?
  • Have I tested the changes (manually)?
    • Are all unit tests still passing? (npm run test)

@flx-sta flx-sta added Localization Provides or updates translation efforts Refactor Rewriting existing code related labels Sep 24, 2024
@flx-sta flx-sta marked this pull request as ready for review September 24, 2024 17:41
@flx-sta flx-sta requested review from a team as code owners September 24, 2024 17:41
@flx-sta flx-sta requested review from CodeTappert and a team and removed request for f-fsantos September 24, 2024 17:43
@flx-sta flx-sta requested review from a team and Tempo-anon and removed request for a team and CodeTappert September 24, 2024 17:43
@flx-sta flx-sta merged commit 9af8941 into pagefaultgames:beta Sep 24, 2024
14 checks passed
@flx-sta flx-sta deleted the qol/status-effect-none-redundancy branch September 24, 2024 21:15
Lneacx pushed a commit to Lneacx/pokerogue that referenced this pull request Sep 25, 2024
… i18n (pagefaultgames#4405)

* remove: `StatusEffect.NONE` from displaying any messages

even thougn this case will never occur, by code definition it should be covered

* remove: obsolete `statusEffect:none.` translation keys

* fix: status-effect test

* chore: undo overrides commit (whops)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Localization Provides or updates translation efforts Refactor Rewriting existing code related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants