diff --git a/.changeset/percent-formatter-locale-4553.md b/.changeset/percent-formatter-locale-4553.md new file mode 100644 index 000000000..13f3f46c8 --- /dev/null +++ b/.changeset/percent-formatter-locale-4553.md @@ -0,0 +1,64 @@ +--- +'@object-ui/fields': minor +'@object-ui/plugin-dashboard': minor +'@object-ui/plugin-gantt': patch +'@object-ui/plugin-grid': patch +--- + +`formatPercent` groups its output and follows the display locale — the last +tooltip/cell channel (objectui#4553). + +PR #4557 threaded the gantt tooltip's number and currency rows and measured that +the percent row could not follow: `formatPercent(value, precision)` took no +locale parameter, and its whole body was +`${percentDisplayValue(value).toFixed(precision)}%`. It built no +`Intl.NumberFormat` and never reached `formatDisplayNumber` — so unlike its +siblings it did not render in the MACHINE's locale, it rendered in **no** locale: +an ASCII decimal mark, never a grouping separator, byte-identical on every +machine. + +**English output MOVES, and that is the fix.** Because the function never +grouped, `1235%` was wrong in en-US too, not only in German. Grouping and locale +therefore land together: + +| | before | after | +|---|---|---| +| en, 1234.5 | `1235%` | `1,235%` | +| de, 1234.5 | `1235%` | `1.235\u00a0%` | +| de, 80 | `80%` | `80\u00a0%` | + +Values below the grouping threshold are unchanged in English (`80%`, `12.5%`, +`33.33%`), so the move is confined to four digits and up. German changes at every +magnitude, because the no-break space before the sign is part of the locale's +percent convention — which is what routing through `Intl` buys over appending a +literal `%`. + +The scaling contract is untouched: `percentDisplayValue` still disambiguates a +fraction-stored percent (`0.8` → 80%) from a whole one, so the list cell and the +dashboard measure formatter still agree. + +Consumers are threaded in the same change, the parameter never landing +speculatively: + +- **fields** — `PercentCellRenderer`, on BOTH of its paths. Its whole-percent + branch (`progress` / `completion` fields, which store 0-100 and must skip the + fraction scaling) was a second bare `toFixed` call; leaving it behind would + have made one grid internally inconsistent, so both branches now share one + locale-aware body and differ only in the scaling policy. +- **plugin-gantt** — the tooltip percent row, completing objectui#4553's switch. +- **plugin-grid** — the mobile card's percent cell, which sits in the same + density row as a date cell objectui#4272 had already localized. +- **plugin-dashboard** — `renderFieldValue`'s percent branch. It is a plain + function rather than a component, so it takes the locale as an optional fourth + parameter beside the `tenantCurrency` already threaded that way, and both of + its callers pass it and declare it in their memo dependency arrays. + +Bumps follow each package's own `.d.ts` diff, measured in both directions. +`@object-ui/fields` and `@object-ui/plugin-dashboard` are `minor` on the +objectui#4272 / PR #4544 precedent — quoted from that changeset: "`@object-ui/fields` +is `minor` because `formatDateTime`'s new optional parameter is visible in the +package's entry `.d.ts`; the plugin packages' own `.d.ts` files are +byte-identical, so their change is module-local." Here `formatPercent` and +`renderFieldValue` each gain an entry-visible optional parameter, while +plugin-gantt's and plugin-grid's `.d.ts` files are byte-identical and stay +`patch`. diff --git a/packages/fields/src/__tests__/PercentCellRenderer.locale.test.tsx b/packages/fields/src/__tests__/PercentCellRenderer.locale.test.tsx new file mode 100644 index 000000000..270f40d5d --- /dev/null +++ b/packages/fields/src/__tests__/PercentCellRenderer.locale.test.tsx @@ -0,0 +1,152 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4553 phase 2 — the percent CELL, the fields-internal consumer of + * `formatPercent`. + * + * `PercentCellRenderer` had two rendering paths and BOTH were outside the + * display-locale channel: + * + * const formatted = isWholePercentField + * ? `${numValue.toFixed(precision)}%` // a second bare toFixed path + * : formatPercent(numValue, precision); // no locale to pass + * + * The branch exists for a real reason — a field named `progress` / `completion` + * stores 0-100, so it must NOT go through `percentDisplayValue`'s fraction + * scaling — but it had quietly become a second place where a percent was + * formatted, and it was the more primitive of the two. Threading only the + * `formatPercent` half would have made ONE grid internally inconsistent: a + * `progress` column ungrouped and unlocalized beside a `rate` column that was + * neither. So both branches now render through the same locale-aware body and + * differ only in the scaling policy, which is all the branch was ever about. + * + * ── Directions, predicted in writing BEFORE the run ────────────────────── + * Runner machine locale en-US. + * + * de, ordinary percent 1234.5 `1235%` → `1.235 %` RED + * de, `progress` 1234.5 `1235%` → `1.235 %` RED (the second path) + * en, ordinary percent 1234.5 `1235%` → `1,235%` RED (grouping move) + * en, `progress` 1234.5 `1235%` → `1,235%` RED (grouping move) + * the fraction/whole SPLIT itself PIN, green both sides + * small-value en output PIN, green both sides + * + * Provider-mounted by construction (objectui#4514) — the pure-function cases + * live in `percent-formatter-locale-4553.test.ts`. + */ + +import React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { I18nProvider, LocalizationProvider } from '@object-ui/i18n'; +import { PercentCellRenderer } from '../index'; + +/** German writes a NO-BREAK SPACE (U+00A0) before the percent sign. */ +const NBSP = '\u00a0'; + +/** + * A session: the UI language the user picked plus the tenant's regional + * default. `useDisplayLocale()` resolves tenant regional default → active UI + * language → 'en'. + */ +function renderCell( + value: unknown, + field: Record, + language: string, + tenantLocale?: string, +) { + return render( + + + + + , + ); +} + +/** The cell's text, read off the value span beside the decorative bar. */ +function cellText(): string { + return document.body.textContent ?? ''; +} + +afterEach(() => cleanup()); + +describe('PercentCellRenderer follows the display locale (objectui#4553)', () => { + it('de renders the German percent form', () => { + renderCell(1234.5, { name: 'win_rate' }, 'de'); + expect(cellText()).toContain(`1.235${NBSP}%`); + }); + + /** + * The SECOND path — `progress` matches WHOLE_PERCENT_FIELD_PATTERN, so this + * value skips fraction scaling and used to skip `formatPercent` entirely. + */ + it('de renders the German form on the whole-percent path too', () => { + renderCell(1234.5, { name: 'progress' }, 'de'); + expect(cellText()).toContain(`1.235${NBSP}%`); + }); + + it('en groups at four digits on both paths — the deliberate output move', () => { + const { unmount } = renderCell(1234.5, { name: 'win_rate' }, 'en'); + expect(cellText()).toContain('1,235%'); + unmount(); + + renderCell(1234.5, { name: 'progress' }, 'en'); + expect(cellText()).toContain('1,235%'); + }); + + it('an explicit tenant locale outranks the active UI language', () => { + renderCell(1234.5, { name: 'win_rate' }, 'en', 'de'); + expect(cellText()).toContain(`1.235${NBSP}%`); + expect(cellText()).not.toContain('1,235%'); + }); +}); + +describe('PercentCellRenderer keeps its scaling contract (objectui#4553 must-not-change)', () => { + /** + * PIN, green both sides — and the reason the whole-percent branch was kept + * rather than collapsed into `formatPercent`. The same stored number means + * different things in the two columns, and that must not have changed. + */ + it('an ordinary percent scales a fraction; a progress field does not', () => { + const { unmount } = renderCell(0.5, { name: 'win_rate' }, 'en'); + // Fraction-stored: 0.5 → 50%. + expect(cellText()).toContain('50%'); + unmount(); + + renderCell(0.5, { name: 'progress' }, 'en'); + // Whole-percent field: 0.5 really is half a percent, rounded to 1% at + // precision 0 — NOT 50%. + expect(cellText()).toContain('1%'); + expect(cellText()).not.toContain('50%'); + }); + + /** PIN: small-value English output is byte-identical across the change. */ + it('en small-value output is unchanged (must-not-change)', () => { + renderCell(33.33, { name: 'win_rate', precision: 2 }, 'en'); + expect(cellText()).toContain('33.33%'); + }); + + /** PIN: the decorative bar still reports the un-formatted magnitude. */ + it('the progressbar still carries the numeric value', () => { + renderCell(33.33, { name: 'win_rate' }, 'en'); + expect(screen.getByRole('progressbar')).toHaveAttribute('aria-valuenow', '33.33'); + }); + + /** PIN: the null path is untouched — and the hook runs before it. */ + it('renders the empty value for null without crashing on the hook', () => { + expect(() => renderCell(null, { name: 'win_rate' }, 'de')).not.toThrow(); + }); +}); diff --git a/packages/fields/src/__tests__/percent-formatter-locale-4553.test.ts b/packages/fields/src/__tests__/percent-formatter-locale-4553.test.ts new file mode 100644 index 000000000..7b4b0eec6 --- /dev/null +++ b/packages/fields/src/__tests__/percent-formatter-locale-4553.test.ts @@ -0,0 +1,147 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4553 phase 2 — `formatPercent` was the last formatter outside the + * display-locale channel, and it was outside it in a different way from its + * siblings. + * + * PR #4557 threaded the gantt tooltip's number and currency rows and measured + * that percent could not follow: `formatPercent(value, precision)` took no + * locale parameter, and its whole body was + * `${percentDisplayValue(value).toFixed(precision)}%`. It built no + * `Intl.NumberFormat` and never reached `formatDisplayNumber`, so its output + * was not the MACHINE's locale (the defect its siblings had) but NO locale at + * all: an ASCII decimal mark, never a grouping separator, byte-identical on + * every machine on earth. + * + * That is why grouping and locale land together here. `1235%` is not merely + * un-German — it is wrong in en-US too, where the number is `1,235%`. So the + * English output MOVES, and that move is the fix, not a regression. + * + * ── Directions, predicted in writing BEFORE the run, then measured ─────── + * Runner: node v22.22.2 / ICU 78.2 / machine locale en-US. + * + * en 1234.5 p0 `1235%` → `1,235%` RED — the grouping move + * de 1234.5 p0 `1235%` → `1.235 %` RED — separators inverted + NBSP + * de 80 p0 `80%` → `80 %` RED — de writes a no-break space + * before the sign; en does not + * de 12.5 p1 `12.5%` → `12,5 %` RED — the ruling's named form + * en 80 p0 `80%` → `80%` PIN, green both sides + * en 12.5 p1 `12.5%` → `12.5%` PIN, green both sides + * fraction scaling (0.8 → 80%) PIN, green both sides + * + * The `en` pins are byte-identical because `en` and the runner's `en-US` agree + * and because grouping only shows from four digits up — they are NOT evidence + * the fix works; the `de` cases and the en GROUPING case carry that. + * + * Provider-less by construction (objectui#4514): these are pure-function cases + * that touch no hook. The provider-mounted half of this card lives in + * `PercentCellRenderer.locale.test.tsx`. + */ + +import { describe, it, expect } from 'vitest'; +import { formatPercent } from '../index'; + +/** + * German puts a NO-BREAK SPACE (U+00A0) between the number and the percent + * sign. Written as an escape rather than pasted, so the expectation is + * readable and greppable in both spellings. + */ +const NBSP = '\u00a0'; + +describe('formatPercent groups its output (objectui#4553)', () => { + /** + * THE en RED CASE, and the one that justifies calling this a defect rather + * than a localization nicety: four digits went out ungrouped in English too. + */ + it('en groups from four digits up — the output MOVES, and that is the fix', () => { + expect(formatPercent(1234.5, 0, 'en')).toBe('1,235%'); + expect(formatPercent(1000000, 0, 'en')).toBe('1,000,000%'); + }); + + /** PIN, green both sides: below the grouping threshold nothing changes. */ + it('en output below the grouping threshold is byte-identical (must-not-change)', () => { + expect(formatPercent(80, 0, 'en')).toBe('80%'); + expect(formatPercent(12.5, 1, 'en')).toBe('12.5%'); + expect(formatPercent(33.33, 2, 'en')).toBe('33.33%'); + expect(formatPercent(0, 0, 'en')).toBe('0%'); + }); +}); + +describe('formatPercent follows the display locale (objectui#4553)', () => { + /** + * The separators invert AND a no-break space appears before the sign, so the + * German form cannot coincide with the machine's on this runner. + */ + it('de inverts the separators and spaces the percent sign', () => { + expect(formatPercent(1234.5, 0, 'de')).toBe(`1.235${NBSP}%`); + expect(formatPercent(12.5, 1, 'de')).toBe(`12,5${NBSP}%`); + }); + + /** + * Even with no separator in play, German still differs from English: the + * space before the sign is part of the locale's percent CONVENTION, which is + * what routing through `Intl` buys over appending a literal '%'. + */ + it('de spaces the sign even for a value with no separators', () => { + expect(formatPercent(80, 0, 'de')).toBe(`80${NBSP}%`); + }); + + /** `Intl` accepts `'zh'` verbatim — no mapping table anywhere. */ + it('zh renders its own convention', () => { + expect(formatPercent(1234.5, 0, 'zh')).toBe('1,235%'); + }); + + /** + * A malformed tag from a tenant config must never take a cell down — + * `formatDisplayNumber` catches it and retries without the locale. + */ + it('a malformed locale tag falls back instead of throwing', () => { + expect(() => formatPercent(80, 0, 'not a locale')).not.toThrow(); + expect(formatPercent(80, 0, 'not a locale')).toContain('80'); + }); +}); + +describe('formatPercent keeps its existing contract (objectui#4553 must-not-change)', () => { + /** + * PIN: the fraction/whole disambiguation is `percentDisplayValue`'s, shared + * with the dashboard measure formatter. The locale parameter must not have + * moved it. + */ + it('still scales a fraction-stored percent and passes a whole one through', () => { + expect(formatPercent(0.8, 0, 'en')).toBe('80%'); + expect(formatPercent(0.5, 0, 'en')).toBe('50%'); + expect(formatPercent(0.075, 1, 'en')).toBe('7.5%'); + // >= 1 is already in display magnitude and is NOT scaled again. + expect(formatPercent(80, 0, 'en')).toBe('80%'); + expect(formatPercent(100, 0, 'en')).toBe('100%'); + }); + + it('still honors the precision it is given', () => { + expect(formatPercent(33.333, 0, 'en')).toBe('33%'); + expect(formatPercent(33.333, 1, 'en')).toBe('33.3%'); + expect(formatPercent(33.333, 2, 'en')).toBe('33.33%'); + }); + + it('still handles negatives', () => { + expect(formatPercent(-45.5, 1, 'en')).toBe('-45.5%'); + }); + + /** + * The parameter is OPTIONAL and third, matching `formatNumber(value, + * decimals, locale)` and `formatCurrency(value, currency, locale)`. An + * existing caller passing nothing still gets the runtime default locale — + * though it now also gets grouping, which is the deliberate output move. + */ + it('is callable with no locale, and with no precision either', () => { + expect(() => formatPercent(80)).not.toThrow(); + expect(formatPercent(80)).toBe('80%'); + expect(formatPercent(80, 0)).toBe('80%'); + }); +}); diff --git a/packages/fields/src/index.tsx b/packages/fields/src/index.tsx index f754751e9..a98c8de95 100644 --- a/packages/fields/src/index.tsx +++ b/packages/fields/src/index.tsx @@ -467,14 +467,55 @@ export function formatNumber(value: number, decimals: number = 2, locale?: strin } /** - * Format percent value - * Handles both decimal (0.8 = 80%) and whole number (80 = 80%) inputs. + * The percent rendering itself, on a value ALREADY in display magnitude (`80` + * means 80%). + * + * Split out from {@link formatPercent} because `PercentCellRenderer`'s + * whole-percent branch needs this exact rendering under a DIFFERENT scaling + * policy (see there). Two copies of the expression is precisely the drift + * `percentDisplayValue`'s doc comment exists to prevent, so there is one copy + * and the scaling decision is made by the caller. */ -export function formatPercent(value: number, precision: number = 0): string { +function formatPercentBody(displayValue: number, precision: number, locale?: string): string { + try { + // `style: 'percent'` multiplies by 100, so the display magnitude is divided + // back out. Going through `Intl` rather than appending a literal '%' is what + // buys the locale's percent CONVENTION and not merely its separators: + // German writes `1.235 %` with a no-break space before the sign, English + // `1,235%` with none. Both bounds are set to `precision` so the width is + // exactly the one the caller asked for — the same contract `toFixed` gave. + return formatDisplayNumber(displayValue / 100, { + locale, + style: 'percent', + minimumFractionDigits: precision, + maximumFractionDigits: precision, + }); + } catch { + return `${displayValue.toFixed(precision)}%`; + } +} + +/** + * Format percent value. + * Handles both decimal (0.8 = 80%) and whole number (80 = 80%) inputs. + * + * `locale` is the third positional parameter, matching {@link formatNumber} and + * {@link formatCurrency} — the shape the sibling formatters already use. + * Callers should pass the tag from `useDisplayLocale()`. + * + * Before objectui#4553 this function took no locale and never touched `Intl`: + * its whole body was `${percentDisplayValue(value).toFixed(precision)}%`, so it + * rendered in NO locale rather than the machine's — an ASCII decimal mark and + * never a grouping separator, byte-identical on every machine. That made + * `1235%` the output everywhere, which is wrong in en-US as well as in German, + * so the grouping and the locale are fixed together: en output MOVES from + * `1235%` to `1,235%` at four digits and up, and that move is the fix. + */ +export function formatPercent(value: number, precision: number = 0, locale?: string): string { // Scale a fraction-stored percent (0.8 → 80%) via the shared core helper, so // the list cell and the dashboard measure formatter (`formatMeasure`) agree. const displayValue = percentDisplayValue(value); - return `${displayValue.toFixed(precision)}%`; + return formatPercentBody(displayValue, precision, locale); } /** @@ -722,8 +763,12 @@ const WHOLE_PERCENT_FIELD_PATTERN = /progress|completion/; * Percent field cell renderer with mini progress bar */ export function PercentCellRenderer({ value, field }: CellRendererProps): React.ReactElement { + // Hook before the empty-value early return — a value flipping between null + // and set must not change the hook count between renders (same rule as + // NumberCellRenderer / CurrencyCellRenderer above). + const locale = useDisplayLocale(); if (value == null) return ; - + const safe = coerceToSafeValue(value); const percentField = field as any; const precision = percentField.precision ?? 0; @@ -737,7 +782,16 @@ export function PercentCellRenderer({ value, field }: CellRendererProps): React. const barValue = isWholePercentField ? numValue : (numValue > -1 && numValue < 1) ? numValue * 100 : numValue; - const formatted = isWholePercentField ? `${numValue.toFixed(precision)}%` : formatPercent(numValue, precision); + // Both branches render through the same locale-aware body (objectui#4553); + // they differ ONLY in the scaling policy, which is the whole point of the + // branch. The whole-percent branch used to be a second bare `toFixed` path, + // so before this card a `progress` field was ungrouped and unlocalized even + // where an ordinary percent column would not have been — leaving it behind + // would have made ONE grid internally inconsistent, which is worse than the + // uniform defect it had. + const formatted = isWholePercentField + ? formatPercentBody(numValue, precision, locale) + : formatPercent(numValue, precision, locale); const clampedBar = Math.max(0, Math.min(100, barValue)); // Layout contract (objectstack#5066): THE NUMBER IS THE CONTENT, THE BAR IS diff --git a/packages/plugin-dashboard/src/ObjectDataTable.tsx b/packages/plugin-dashboard/src/ObjectDataTable.tsx index 91bf68691..31874f930 100644 --- a/packages/plugin-dashboard/src/ObjectDataTable.tsx +++ b/packages/plugin-dashboard/src/ObjectDataTable.tsx @@ -11,7 +11,7 @@ import { useDataScope, SchemaRendererContext, SchemaRenderer, useFilterScope } f import { extractRecords, isDrillEnabled } from '@object-ui/core'; import type { DrillDownConfig } from '@object-ui/types'; import { Skeleton, RefreshIndicator, cn } from '@object-ui/components'; -import { useSafeFieldLabel, useObjectTranslation, useLocalization } from '@object-ui/i18n'; +import { useSafeFieldLabel, useObjectTranslation, useLocalization, useDisplayLocale } from '@object-ui/i18n'; import { resolveFilterPlaceholders } from './utils'; import { buildFieldMeta, @@ -135,6 +135,9 @@ export function computeLookupExpand( export const ObjectDataTable: React.FC = ({ schema, dataSource: propDataSource, className }) => { // Tenant default currency backstops columns that omit an explicit code. const { currency: tenantCurrency } = useLocalization(); + // objectui#4553: percent/number cells are FORMATTED inside the memo below, + // so the locale is both an argument and a dependency of it. + const displayLocale = useDisplayLocale(); const context = useContext(SchemaRendererContext); const dataSource = propDataSource || context?.dataSource; const boundData = useDataScope(schema.bind); @@ -310,7 +313,7 @@ export const ObjectDataTable: React.FC = ({ schema, dataSo if (typeof col.cell === 'function') return { ...col, ...fieldMeta, align: inferredAlign }; // Tenant-default currency backstops a currency column with no explicit code. - const cell = (value: any): React.ReactNode => renderFieldValue(value, fieldMeta, tenantCurrency); + const cell = (value: any): React.ReactNode => renderFieldValue(value, fieldMeta, tenantCurrency, displayLocale); return { ...col, ...fieldMeta, align: inferredAlign, cell }; }; @@ -334,7 +337,7 @@ export const ObjectDataTable: React.FC = ({ schema, dataSo : Object.keys(finalData[0]).filter((k) => !k.startsWith('_') && !isSystemField(k)); return orderedKeys.map((k) => enrich({ header: buildHeader(k), accessorKey: k })); - }, [schema.columns, schema.objectName, finalData, objectSchema, fieldLabel, fieldOptionLabel, tenantCurrency]); + }, [schema.columns, schema.objectName, finalData, objectSchema, fieldLabel, fieldOptionLabel, tenantCurrency, displayLocale]); // Note: per-cell select-label translation that used to happen here is now // handled by SelectCellRenderer in the shared field registry, which also diff --git a/packages/plugin-dashboard/src/RecordDetailDrawer.tsx b/packages/plugin-dashboard/src/RecordDetailDrawer.tsx index b90de7c7e..ee8299526 100644 --- a/packages/plugin-dashboard/src/RecordDetailDrawer.tsx +++ b/packages/plugin-dashboard/src/RecordDetailDrawer.tsx @@ -25,7 +25,7 @@ import { Sheet, SheetContent, SheetHeader, SheetTitle, Dialog, DialogContent, DialogHeader, DialogTitle, } from '@object-ui/components'; -import { useSafeFieldLabel, useLocalization } from '@object-ui/i18n'; +import { useSafeFieldLabel, useLocalization, useDisplayLocale } from '@object-ui/i18n'; import { indexObjectFields, buildFieldMeta, @@ -80,6 +80,8 @@ export const RecordDetailDrawer: React.FC = ({ }) => { const { fieldLabel, fieldOptionLabel } = useSafeFieldLabel(); const { currency: tenantCurrency } = useLocalization(); + // objectui#4553: see ObjectDataTable — formatted in the memo, so it is a dep. + const displayLocale = useDisplayLocale(); const rows = useMemo(() => { if (!record) return []; @@ -105,11 +107,11 @@ export const RecordDetailDrawer: React.FC = ({ return { key, label, - node: renderFieldValue(record[key], fieldMeta, tenantCurrency), + node: renderFieldValue(record[key], fieldMeta, tenantCurrency, displayLocale), numeric: isNumericFieldMeta(fieldMeta), }; }); - }, [record, objectSchema, objectName, fields, fieldLabel, fieldOptionLabel, tenantCurrency]); + }, [record, objectSchema, objectName, fields, fieldLabel, fieldOptionLabel, tenantCurrency, displayLocale]); if (!record) return null; diff --git a/packages/plugin-dashboard/src/__tests__/ObjectDataTable.percentLocale.test.tsx b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.percentLocale.test.tsx new file mode 100644 index 000000000..0083412f9 --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.percentLocale.test.tsx @@ -0,0 +1,216 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4553 phase 2 — the dashboard's percent-formatted cells. + * + * `renderFieldValue` in `recordFields.tsx` formats a `%`-format column through + * `formatPercent` and threaded no locale, because there was none to thread. + * Unlike the other consumers on this card it is a PLAIN FUNCTION, not a + * component, so it cannot read `useDisplayLocale()` itself — the locale has to + * arrive as an argument. It is given an optional fourth parameter beside the + * `tenantCurrency` that was already threaded exactly this way. + * + * ⚠️ Both of its callers format INSIDE a `useMemo`, so the locale was added to + * their dependency arrays as well — objectui#4542's lesson (PR #4554) applied + * in a second place. See the honest-scope note on the last case: that + * dependency turned out NOT to be assertable from here, and the note says so + * rather than letting the case read as a pin it is not. + * + * Note the DASHBOARD MEASURE formatter is a different thing that this card + * does NOT touch: `formatMeasure` in `@object-ui/core` has its own percent + * path (`toLocaleString(undefined, ...)`) and never calls `formatPercent`, so + * `DatasetWidget` / `ObjectMetric` are out of this change's blast radius. + * + * ── Directions, predicted in writing BEFORE the run ────────────────────── + * Runner machine locale en-US. + * de `1235%` → `1.235 %` RED + * en `1235%` → `1,235%` RED (the deliberate grouping move) + * small-value en output PIN, green both sides + * re-render on a late locale RED pre-fix (formatting), but it + * does NOT isolate the memo dep — + * see its own note + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { I18nProvider, LocalizationProvider, type LocalizationValue } from '@object-ui/i18n'; + +vi.mock('@object-ui/react', async () => { + const actual: any = await vi.importActual('@object-ui/react'); + return { + ...actual, + SchemaRenderer: ({ schema }: any) => { + const cols = schema.columns || []; + const rows = schema.data || []; + return ( + + + {rows.map((row: any, i: number) => ( + + {cols.map((c: any) => ( + + ))} + + ))} + +
+ {typeof c.cell === 'function' + ? c.cell(row[c.accessorKey], row) + : String(row[c.accessorKey] ?? '')} +
+ ); + }, + useDataScope: () => undefined, + SchemaRendererContext: + actual.SchemaRendererContext || + (await vi.importActual('react')).createContext({}), + }; +}); + +import { ObjectDataTable } from '../ObjectDataTable'; + +/** German writes a NO-BREAK SPACE (U+00A0) before the percent sign. */ +const NBSP = '\u00a0'; + +/** Four digits, so the GROUPING separator is exercised in both locales. */ +const ROWS = [{ id: '1', name: 'Northwind', win_rate: 1234.5, small_rate: 33.33 }]; + +const OBJECT_SCHEMA = { + fields: { + name: { type: 'text', label: 'Name' }, + // `format` carrying a '%' is what routes a column through the percent + // branch of `renderFieldValue`. + win_rate: { type: 'percent', label: 'Win Rate', format: '0%' }, + small_rate: { type: 'percent', label: 'Small Rate', format: '0.00%' }, + }, +}; + +/** + * MODULE-CONSTANT, and that is load-bearing for the dependency case at the + * bottom of this file — objectui#4542 / PR #4554's masking-path lesson, met + * again here. + * + * Building a fresh data source per render (the `dataSource={makeDataSource()}` + * this file started with) gives it a new identity on the re-render, which + * refetches, which gives + * `finalData` a new identity, which re-runs the column memo ALL BY ITSELF. The + * dependency case then passes whether or not `displayLocale` is in the memo's + * dependency array — i.e. it looks like coverage and is not. Measured: with the + * dep deliberately removed and a per-render source, all five cases still passed. + * A stable identity is what makes the locale the only thing that can invalidate + * that memo. + */ +const DATA_SOURCE: any = { + find: vi.fn(async () => ({ data: ROWS, total: ROWS.length })), + getObjectSchema: vi.fn(async () => OBJECT_SCHEMA), +}; + +const SCHEMA: any = { + type: 'object-data-table', + objectName: 'accounts', + columns: [ + { accessorKey: 'name', header: 'Name' }, + { accessorKey: 'win_rate', header: 'Win Rate' }, + { accessorKey: 'small_rate', header: 'Small Rate' }, + ], +}; + +function renderSession(language: string, value: LocalizationValue = {}) { + return render( + + + + + , + ); +} + +afterEach(() => cleanup()); + +describe('ObjectDataTable percent cells follow the display locale (objectui#4553)', () => { + it('de session renders the German percent form', async () => { + renderSession('de'); + await waitFor(() => expect(screen.getByTestId('cell-win_rate')).toBeInTheDocument()); + expect(screen.getByTestId('cell-win_rate').textContent).toBe(`1.235${NBSP}%`); + }); + + it('en session groups the percent — the deliberate output move', async () => { + renderSession('en'); + await waitFor(() => expect(screen.getByTestId('cell-win_rate')).toBeInTheDocument()); + expect(screen.getByTestId('cell-win_rate').textContent).toBe('1,235%'); + }); + + /** PIN, green both sides: below the grouping threshold en is unchanged. */ + it('en small-value output is byte-identical (must-not-change)', async () => { + renderSession('en'); + await waitFor(() => expect(screen.getByTestId('cell-small_rate')).toBeInTheDocument()); + expect(screen.getByTestId('cell-small_rate').textContent).toBe('33.33%'); + }); + + it('an explicit tenant locale outranks the active UI language', async () => { + renderSession('en', { locale: 'de' }); + await waitFor(() => expect(screen.getByTestId('cell-win_rate')).toBeInTheDocument()); + expect(screen.getByTestId('cell-win_rate').textContent).toBe(`1.235${NBSP}%`); + }); + + /** + * The user-visible guarantee: a locale arriving after first paint re-formats + * the cells rather than leaving them on the old convention. + * + * ⚠️ HONEST SCOPE — this case asserts the BEHAVIOR, and it does NOT isolate + * the memo dependency, though it was written intending to. Measured both + * ways: with `displayLocale` deliberately removed from the column memo's + * dependency array (and the argument still threaded), all five cases in this + * file still passed. The provider change makes this component refetch, which + * gives `finalData` a fresh identity and re-runs the memo on its own — so no + * assertion here can distinguish a declared dependency from an undeclared + * one. Making the data source module-constant (above) removes one masking + * path but not that one. + * + * The dependency is declared in `ObjectDataTable` anyway, and should stay: + * it is what `react-hooks/exhaustive-deps` requires, and it is what keeps the + * cells correct if the refetch ever stops coinciding with the locale change. + * But it is currently guarded by reasoning, not by this test — recorded here + * so a later reader does not mistake this case for the pin it is not. + */ + it('re-formats when the tenant locale changes after first paint', async () => { + const { rerender } = render( + + + + + , + ); + await waitFor(() => expect(screen.getByTestId('cell-win_rate')).toBeInTheDocument()); + expect(screen.getByTestId('cell-win_rate').textContent).toBe('1,235%'); + + rerender( + + + + + , + ); + + await waitFor(() => + expect(screen.getByTestId('cell-win_rate').textContent).toBe(`1.235${NBSP}%`), + ); + }); +}); diff --git a/packages/plugin-dashboard/src/recordFields.tsx b/packages/plugin-dashboard/src/recordFields.tsx index 9ba63033b..0a959f546 100644 --- a/packages/plugin-dashboard/src/recordFields.tsx +++ b/packages/plugin-dashboard/src/recordFields.tsx @@ -169,6 +169,11 @@ export function renderFieldValue( value: any, fieldMeta: FieldMeta, tenantCurrency?: string, + // BCP-47 display locale from `useDisplayLocale()` (objectui#4553). Optional + // and last, so an existing caller that passes nothing keeps the behavior it + // had. This is a plain function rather than a component, so the locale has to + // arrive as an argument — there is no hook to read it from here. + displayLocale?: string, ): React.ReactNode { if (value == null || value === '') return ''; const fmt = fieldMeta.format; @@ -183,7 +188,7 @@ export function renderFieldValue( if (typeof fmt === 'string' && /%/.test(fmt) && typeof value === 'number') { const decimals = (fmt.match(/0\.(0+)%/) || [undefined, ''] as any)[1].length; const normalized = value > 1 ? value / 100 : value; - return formatPercent(normalized * 100, decimals); + return formatPercent(normalized * 100, decimals, displayLocale); } if (typeof fmt === 'string' && /[YMDHms]/.test(fmt)) { return formatDate(value, fmt); diff --git a/packages/plugin-gantt/src/ObjectGantt.numberLocale.test.tsx b/packages/plugin-gantt/src/ObjectGantt.numberLocale.test.tsx index e90bf20b3..a83cd897b 100644 --- a/packages/plugin-gantt/src/ObjectGantt.numberLocale.test.tsx +++ b/packages/plugin-gantt/src/ObjectGantt.numberLocale.test.tsx @@ -24,6 +24,21 @@ * `1.234,50`. The separators are INVERTED, so the amount is not merely * unstyled — it reads as a different number. * + * ── Phase 2 update (objectui#4553): the percent row is now threaded too ── + * The inversion described below was resolved at the PRODUCER: `formatPercent` + * gained an optional locale (parameter 3, matching its siblings) and now routes + * through `formatDisplayNumber`. Two expectations in this file MOVED as a + * result, both deliberately and under a ruling: + * + * - the `de` percent case, which asserted the un-localized `1235%` as + * evidence of the inversion, now asserts `1.235 %`; and + * - the `en` percent expectation, which is the one thing in the `en` case + * that is NOT byte-identical — `formatPercent` never grouped, so `1235%` + * was wrong in en-US as well, and grouping it is the fix. + * + * The historical account below is kept because it is the reason the percent row + * could not land with the other two. + * * ── The card's premise held for two of the three formatters, not three ─── * Measured against `@object-ui/fields` before writing any fix (the signature * measurement objectui#4553's ruling required): @@ -62,8 +77,8 @@ * de currency `EUR1,234.50` → `1.234,50 EUR` RED before, green after * (symbol moves to the END as well as the * separators inverting — doubly un-fakeable) - * de percent `1235%` → `1235%` GREEN BOTH SIDES — the inverted - * premise, pinned as evidence + * de percent `1235%` → `1.235 %` RED as of phase 2 (was the + * inverted-premise pin in #4557) * en number / currency / percent GREEN BOTH SIDES — PINS. `en` and * the runner's `en-US` coincide, so * these assert byte-identical @@ -259,36 +274,49 @@ describe('ObjectGantt tooltips — numeric values follow the display locale (obj expect(screen.getByTestId(QTY).textContent).toBe('Qty=1,234.50'); expect(screen.getByTestId(AMOUNT).textContent).toBe(`Amount=${EURO}1,234.50`); - expect(screen.getByTestId(RATIO).textContent).toBe('Ratio=1235%'); + // MOVED by objectui#4553 phase 2, and the one expectation in this `en` case + // that is NOT byte-identical: `formatPercent` never grouped, so this row + // read `1235%` in en-US too. Grouping it is the fix, not a regression. + expect(screen.getByTestId(RATIO).textContent).toBe('Ratio=1,235%'); expect(screen.getByTestId(DUE).textContent).toBe('Due=Jan 5, 2024'); }); /** - * PIN, green on both sides — and the card's PREMISE INVERSION, recorded in - * the tree rather than only in a report. + * PIN MOVED, deliberately and under a ruling — this case previously asserted + * the OPPOSITE, and the change of expectation is the point. * - * objectui#4553 states that all three numeric formatters reach - * `formatDisplayNumber` with `locale: undefined`. `formatPercent` does not: - * it is `${percentDisplayValue(value).toFixed(precision)}%`, so it touches no - * `Intl` at all and takes no locale parameter to thread. Its output is - * therefore not the machine's locale but NO locale — ASCII, ungrouped, - * identical on every machine. German would want `1.235 %`; every session - * gets `1235%`. + * When PR #4557 landed, this row was the card's measured premise inversion: + * `formatPercent(value, precision)` took no locale, so it rendered in NO + * locale at all (`1235%` on every machine — ungrouped even in en-US), and it + * was pinned that way as evidence rather than as an endorsement, with a note + * that it would go red the day `formatPercent` grew a locale. * - * Fixing it means adding a parameter to a `@object-ui/fields` export, which - * is outside this card's ruled surface — so it is pinned as-is here and - * escalated. This assertion is a change-detector for a known-wrong output, - * NOT a statement that the output is right; it is expected to fail on the - * day `formatPercent` grows a locale, and that failure is the signal to - * update it. + * That day is objectui#4553 phase 2. `formatPercent` now takes a locale as + * its third parameter and routes through `formatDisplayNumber`, so the row + * finally speaks the same convention as the number, currency and date rows + * beside it. The old expectation is retired exactly as its own comment + * predicted. */ - it('de percent stays ASCII and ungrouped — formatPercent takes no locale (objectui#4553 inverted premise)', async () => { + it('de percent follows the display locale (objectui#4553 phase 2 — pin moved)', async () => { renderSession('de'); await waitFor(() => expect(screen.getByTestId('gv-fields-1')).toBeDefined()); - expect(screen.getByTestId(RATIO).textContent).toBe('Ratio=1235%'); - // The German rendering this row cannot currently produce, spelled out so - // the gap is legible without re-deriving it from the formatter. - expect(screen.getByTestId(RATIO).textContent).not.toBe(`Ratio=1.235${NBSP}%`); + expect(screen.getByTestId(RATIO).textContent).toBe(`Ratio=1.235${NBSP}%`); + // The pre-phase-2 rendering, spelled out so the move is legible in place. + expect(screen.getByTestId(RATIO).textContent).not.toBe('Ratio=1235%'); + }); + + /** + * The whole tooltip now speaks ONE convention — the card's original + * complaint, closed. Number, currency, percent and date rows together. + */ + it('every row of one tooltip agrees on the German convention', async () => { + renderSession('de'); + await waitFor(() => expect(screen.getByTestId('gv-fields-1')).toBeDefined()); + + expect(screen.getByTestId(QTY).textContent).toBe('Qty=1.234,50'); + expect(screen.getByTestId(AMOUNT).textContent).toBe(`Amount=1.234,50${NBSP}${EURO}`); + expect(screen.getByTestId(RATIO).textContent).toBe(`Ratio=1.235${NBSP}%`); + expect(screen.getByTestId(DUE).textContent).toBe('Due=5. Jan. 2024'); }); }); diff --git a/packages/plugin-gantt/src/ObjectGantt.tsx b/packages/plugin-gantt/src/ObjectGantt.tsx index 6a683bb0c..efc9f363f 100644 --- a/packages/plugin-gantt/src/ObjectGantt.tsx +++ b/packages/plugin-gantt/src/ObjectGantt.tsx @@ -679,21 +679,15 @@ export const ObjectGantt: React.FC = ({ resolveFieldCurrency(def as any, tenantCurrency), displayLocale, ); - // ⚠️ `percent` is deliberately NOT threaded here, and it is the one row - // in this switch that still ignores the display locale. - // `formatPercent(value, precision)` takes no locale parameter at all: - // it is `${percentDisplayValue(value).toFixed(precision)}%`, so it - // constructs no `Intl.NumberFormat` and never reaches - // `formatDisplayNumber`. Its output is therefore not the machine's - // locale but NO locale — ASCII `.`, never grouped, identical on every - // machine (`1235%` where German wants `1.235 %`). Fixing it means - // growing a `@object-ui/fields` export's signature, which objectui#4553's - // ruled surface excludes; escalated on that card rather than patched - // with a locale-aware reimplementation here, which would fork percent - // formatting away from the list cell and the dashboard measure - // formatter that share `percentDisplayValue` today. + // `percent` completes the switch (objectui#4553 phase 2). It could not + // be threaded when the other two were: `formatPercent` took no locale + // parameter and never touched `Intl`, so it rendered in NO locale at + // all — ASCII `.`, never grouped, `1235%` on every machine. That was + // fixed at the producer rather than reimplemented here, which would + // have forked percent formatting away from the list cell renderer and + // the dashboard that share `percentDisplayValue`. case 'percent': - return formatPercent(Number(value)); + return formatPercent(Number(value), undefined, displayLocale); case 'boolean': case 'checkbox': return value ? 'Yes' : 'No'; diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 8f5470d2a..b2754124e 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -3007,7 +3007,10 @@ export const ObjectGrid: React.FC = ({ )} {percentCols[0] && row[percentCols[0].accessorKey] != null && ( - {formatPercent(Number(row[percentCols[0].accessorKey]))} + {/* objectui#4553: the mobile card's percent cell takes + the same `displayLocale` its date sibling above + already does (objectui#4272). */} + {formatPercent(Number(row[percentCols[0].accessorKey]), undefined, displayLocale)} )} diff --git a/packages/plugin-grid/src/__tests__/mobileCardPercentLocale.test.tsx b/packages/plugin-grid/src/__tests__/mobileCardPercentLocale.test.tsx new file mode 100644 index 000000000..7c9d9158b --- /dev/null +++ b/packages/plugin-grid/src/__tests__/mobileCardPercentLocale.test.tsx @@ -0,0 +1,145 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4553 phase 2 — the mobile card's percent cell. + * + * Below the 768px breakpoint ObjectGrid switches to a stacked card layout. Its + * density row renders a date and a percent SIDE BY SIDE: + * + * {formatDate(row[dateCols[0]...], 'short', { locale: displayLocale })} + * {formatPercent(Number(row[percentCols[0]...]))} + * + * objectui#4272 threaded the date half. The percent half beside it could not be + * threaded then — `formatPercent` had no locale parameter — so one row rendered + * two conventions, the same shape objectui#4553 found in the gantt tooltip. + * + * This consumer was NOT in the card's named consumer list; it came out of the + * repo-wide `formatPercent` census the ruling asked for. + * + * ── Directions, predicted in writing BEFORE the run ────────────────────── + * Runner machine locale en-US. + * de `1235%` → `1.235 %` RED + * en `1235%` → `1,235%` RED (the deliberate grouping move) + * the date half beside it PIN, green both sides + */ + +import React from 'react'; +import { describe, it, expect, beforeAll, afterEach, vi } from 'vitest'; +import { render, screen, waitFor, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { I18nProvider, LocalizationProvider } from '@object-ui/i18n'; +import { ActionProvider, SchemaRendererProvider } from '@object-ui/react'; +import { registerAllFields } from '@object-ui/fields'; +import { ObjectGrid } from '../ObjectGrid'; + +registerAllFields(); + +beforeAll(() => { + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +/** German writes a NO-BREAK SPACE (U+00A0) before the percent sign. */ +const NBSP = '\u00a0'; + +const ORIGINAL_INNER_WIDTH = window.innerWidth; + +function setMobileWidth() { + Object.defineProperty(window, 'innerWidth', { writable: true, configurable: true, value: 390 }); +} + +afterEach(() => { + Object.defineProperty(window, 'innerWidth', { writable: true, configurable: true, value: ORIGINAL_INNER_WIDTH }); + cleanup(); +}); + +/** + * A four-digit percent, so the GROUPING separator is exercised — the whole + * point of the en case. `win_rate` is named to match `classify()`'s + * `percentKeys` ('rate'), which is what routes it to the percent cell at all; + * `close_date` matches `dateKeys` for the same reason. + */ +const ROWS = [ + { id: 'a1', account_name: 'Northwind', close_date: '2024-03-15', win_rate: 1234.5 }, +]; + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: ROWS, total: ROWS.length, hasMore: false, pageSize: 50 })), + getObjectSchema: async (name: string) => ({ + name, + fields: { + id: { type: 'text' }, + account_name: { type: 'text', label: 'Account Name' }, + close_date: { type: 'date', label: 'Close Date' }, + win_rate: { type: 'percent', label: 'Win Rate' }, + }, + }), + } as any; +} + +function renderSession(language: string, tenantLocale?: string) { + const ds = makeDataSource(); + const schema: any = { + type: 'object-grid', + objectName: 'showcase_account', + columns: [ + { field: 'account_name', label: 'Account Name' }, + { field: 'close_date', label: 'Close Date', type: 'date' }, + { field: 'win_rate', label: 'Win Rate', type: 'percent' }, + ], + pagination: { pageSize: 50 }, + }; + return render( + + + + + + + + + , + ); +} + +describe('ObjectGrid mobile cards — the percent cell follows the display locale (objectui#4553)', () => { + it('de session renders the German percent form', async () => { + setMobileWidth(); + const { container } = renderSession('de'); + await waitFor(() => expect(screen.getByText('Northwind')).toBeInTheDocument()); + expect(container.textContent).toContain(`1.235${NBSP}%`); + expect(container.textContent).not.toContain('1235%'); + }); + + it('en session groups the percent — the deliberate output move', async () => { + setMobileWidth(); + const { container } = renderSession('en'); + await waitFor(() => expect(screen.getByText('Northwind')).toBeInTheDocument()); + expect(container.textContent).toContain('1,235%'); + }); + + /** + * The card's actual complaint, asserted as ONE row: the date cell and the + * percent cell beside it now speak the same convention. Pre-fix the date was + * already German (objectui#4272) and the percent was not. + */ + it('the date cell and the percent cell in one row agree on one convention', async () => { + setMobileWidth(); + const { container } = renderSession('de'); + await waitFor(() => expect(screen.getByText('Northwind')).toBeInTheDocument()); + // objectui#4272's date half — PIN, unchanged by this card. + expect(container.textContent).toContain('Mär'); + expect(container.textContent).toContain(`1.235${NBSP}%`); + }); +});