Fixed toLocaleOrdinal by using Intl.PluralRules instead of fixed rules - #98265
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@roryabraham Ready for your review |
| * `other` is required because it is the fallback for any category a locale does not declare, which | ||
| * keeps a missing entry from producing an empty string. | ||
| */ | ||
| type LocaleOrdinals = Partial<Record<Intl.LDMLPluralRule, OrdinalRenderer>> & {other: OrdinalRenderer}; |
There was a problem hiding this comment.
NAB Minor type cleanup:
diff --git a/src/languages/localeOrdinalMap.ts b/src/languages/localeOrdinalMap.ts
index 4eecc665dd7..61477fff882 100644
--- a/src/languages/localeOrdinalMap.ts
+++ b/src/languages/localeOrdinalMap.ts
@@ -14,6 +14,8 @@
import type {Locale} from '@src/CONST/LOCALES';
import {LOCALES} from '@src/CONST/LOCALES';
+import type {SetRequired} from 'type-fest';
+
/** Renders the full ordinal for a number, e.g. `1st`, `1.`, `第1` */
type OrdinalRenderer = (count: number) => string;
@@ -21,7 +23,7 @@ type OrdinalRenderer = (count: number) => string;
* `other` is required because it is the fallback for any category a locale does not declare, which
* keeps a missing entry from producing an empty string.
*/
-type LocaleOrdinals = Partial<Record<Intl.LDMLPluralRule, OrdinalRenderer>> & {other: OrdinalRenderer};
+type LocaleOrdinals = SetRequired<Partial<Record<Intl.LDMLPluralRule, OrdinalRenderer>>, 'other'>;
const localeOrdinalMap: Record<Locale, LocaleOrdinals> = {
/** Categories: one, two, few, other. The only locale here needing all four. */
|
|
||
| /** Categories: other. Uses the masculine ordinal indicator. */ | ||
| [LOCALES.ES]: { | ||
| other: (count) => `${count}º`, |
There was a problem hiding this comment.
NAB follow-up: figure out how to correctly use the feminine ordinal indicator when appropriate.
There was a problem hiding this comment.
@MelvinBot Create a follow-up issue to correctly handle gendered nouns in Spanish, Greek and any other locales that use them (for adjectives, ordinality, and any other grammatical structures you can think of that would be effected by this).
Label it Weekly, assign to @shubham1206agra and myself
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
Created the follow-up issue: #98378
It covers correctly handling grammatical gender across all gendered locales (Spanish, Greek, German, French, Italian, pt-BR, Polish, …) — not just ordinals, but adjectives, articles/determiners, past participles, and any other agreement-based structures that need to match the gender/number of the noun they modify. It's framed as a follow-up to this PR, calls out that the written ordinals added here are locked to one gender per locale, and lists concrete scope (audit gender-dependent copy → mechanism for declaring noun gender + selecting the agreeing form → migrate the hard-coded single-gender strings). |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.53-0 🚀
|
|
🤖 No help site changes required. This PR is a purely internal localization/formatting change: it swaps It only affects how ordinal numbers are rendered (e.g. No draft docs PR was created since there's nothing to update. @shubham1206agra, please confirm you agree no help site updates are needed. If you believe a specific article is affected, point me to it and I'll open the docs PR. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.53-10 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
toLocaleOrdinalwas previously hard-coded to work only with English (i.e:1st,2nd,3rd), but those English concepts don't map cleanly to other languages. Some languages have totally different systems or ordinality.This PR fixes it by using
Intl.PluralRules, which correctly handles ordinality for different locales.Fixed Issues
$ #98089
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-08-11.at.3.13.25.PM.mov
iOS: Native
MacOS: Chrome / Safari
Screen.Recording.2026-08-11.at.2.54.39.PM.mov