Repository navigation
Conversation
… decimal/thousands separators distinct Show unit label (and every other trait checkbox) could not be cleared when traits were persisted in PascalCase. Fixes #1824 (comma allowed as both separators).
…ter in-place format set updates Fixes #1825.
…units; show default unit label placeholder DDMMSS was unreachable: GRAD was offered under degrees and ARC_MINUTE as its own sub-unit. Removes unused internal UnitDescr component; log through Logger.
…, stricter sub-unit tolerance, test gaps - Resolve separator conflicts against the formatter's locale defaults; empty separators never collide; leave the thousands separator alone while grouping is off - Tighten whole-subdivision tolerance to 1e-9 relative - Tests: PascalCase traits for every checkbox, string formatTraits, non-whole large ratios, deterministic option waits
…uth of horizontal directions FormatTypeOption hard-coded Units.REVOLUTION and Units.ARC_DEG, which cannot convert to HORIZONTAL_DIRECTION units, so switching a horizontal-direction format to bearing/azimuth made the formatter throw.
There was a problem hiding this comment.
🟡 Changes recommended
Large fractional ratios can pass the subdivision check, and asynchronous type changes can overwrite newer user edits.
2 open findings
What changed in this PR
Fixes five quantity-format editing defects involving separators, stale formats, trait casing, composite sub-units, and horizontal-direction formats.
Changes:
- Centralizes trait and separator handling.
- Corrects format selection, sub-unit filtering, and direction-unit resolution.
- Adds localization, regression tests, and patch changesets.
| File | Description |
|---|---|
src/test/quantityformat/internal/TraitCheckboxes.test.tsx |
Tests PascalCase traits. |
src/test/quantityformat/internal/ThousandsSeparator.test.tsx |
Tests separator conflicts. |
src/test/quantityformat/internal/ShowTrailingZeros.test.tsx |
Corrects string-trait coverage. |
src/test/quantityformat/internal/FormatUnits.test.tsx |
Tests sub-units and labels. |
src/test/quantityformat/internal/FormatUnitLabel.test.tsx |
Tests unit-label traits. |
src/test/quantityformat/internal/FormatType.test.tsx |
Tests direction units. |
src/test/quantityformat/internal/FormatPropsUtils.test.ts |
Tests shared utilities. |
src/test/quantityformat/internal/DecimalSeparator.test.tsx |
Tests decimal conflicts. |
src/test/quantityformat/FormatSelector.test.tsx |
Tests current format retrieval. |
src/components/quantityformat/panels/Station.tsx |
Uses shared trait and unit logic. |
src/components/quantityformat/panels/Scientific.tsx |
Uses shared trait and unit logic. |
src/components/quantityformat/panels/Ratio.tsx |
Uses shared trait and unit logic. |
src/components/quantityformat/panels/Fractional.tsx |
Uses shared trait and unit logic. |
src/components/quantityformat/panels/Decimal.tsx |
Uses shared trait and unit logic. |
src/components/quantityformat/panels/Bearing.tsx |
Supplies the units provider. |
src/components/quantityformat/panels/Azimuth.tsx |
Uses shared trait and unit logic. |
src/components/quantityformat/internal/ZeroEmpty.tsx |
Centralizes trait updates. |
src/components/quantityformat/internal/ThousandsSeparator.tsx |
Resolves grouping conflicts. |
src/components/quantityformat/internal/ShowTrailingZeros.tsx |
Centralizes trait updates. |
src/components/quantityformat/internal/misc/UnitDescr.tsx |
Removes unused component code. |
src/components/quantityformat/internal/misc/UnitDescr.ts |
Retains the unit-name helper. |
src/components/quantityformat/internal/misc/FormatType.tsx |
Resolves direction-family units. |
src/components/quantityformat/internal/KeepSingleZero.tsx |
Centralizes trait updates. |
src/components/quantityformat/internal/KeepDecimalPoint.tsx |
Centralizes trait updates. |
src/components/quantityformat/internal/FractionDash.tsx |
Centralizes trait updates. |
src/components/quantityformat/internal/FormatUnits.tsx |
Filters sub-units and improves labels. |
src/components/quantityformat/internal/FormatUnitLabel.tsx |
Handles trait casing. |
src/components/quantityformat/internal/FormatPropsUtils.ts |
Adds shared format utilities. |
src/components/quantityformat/internal/DecimalSeparator.tsx |
Resolves decimal conflicts. |
src/components/quantityformat/internal/AzimuthOptions.tsx |
Uses structured logging. |
src/components/quantityformat/FormatSelector.tsx |
Reads current format definitions. |
public/locales/en/QuantityFormat.json |
Adds the empty-search translation. |
.changeset/@itwin-quantity-formatting-react-stale-format-selection.md |
Documents stale-format fix. |
.changeset/@itwin-quantity-formatting-react-horizontal-direction-bearing.md |
Documents direction-unit fix. |
.changeset/@itwin-quantity-formatting-react-format-trait-casing.md |
Documents trait-casing fix. |
.changeset/@itwin-quantity-formatting-react-distinct-separators.md |
Documents separator fix. |
.changeset/@itwin-quantity-formatting-react-composite-sub-units.md |
Documents sub-unit fix. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
…ups, cap sub-unit tolerance - FormatTypeOption ignores a bearing/azimuth unit lookup that resolves after a newer type change or props change - Cap the whole-subdivision tolerance at 1e-3 parts so very large non-integral ratios are rejected
| // Not memoized: format set providers (e.g. FormatSetFormatsProvider.addFormat) update `formats` in place, so the | ||
| // object identity does not change after a format is applied. | ||
| const formatEntries = Object.entries(activeFormatSet?.formats ?? {}) | ||
| .filter((entry): entry is [string, FormatDefinition] => typeof entry[1] === "object" && entry[1] !== null) | ||
| .map(([key, formatDef]) => ({ key, formatDef, label: formatDef.label || key })); | ||
|
|
||
| return Object.entries(activeFormatSet.formats) | ||
| .filter(([, formatDef]) => typeof formatDef === "object" && formatDef !== null) | ||
| .map(([key, formatDef]) => ({ | ||
| key, | ||
| formatDef: formatDef as FormatDefinition, | ||
| label: (formatDef as FormatDefinition).label || key, | ||
| })); | ||
| }, [activeFormatSet?.formats]); | ||
| const lowerSearchTerm = searchTerm.trim().toLowerCase(); | ||
| const filteredFormats = lowerSearchTerm | ||
| ? formatEntries.filter(({ label }) => label.toLowerCase().includes(lowerSearchTerm)) | ||
| : formatEntries; |
There was a problem hiding this comment.
If FormatSetFormatsProvider changes formats in place, there's nothing that would cause FormatSelector component to rerender and cause the format updates to be reflected. In addition, React components by design require props to be immutable, and that should be taken care of at the source rather than in a low level component.
IMO, whoever has access to FormatSetFormatsProvider and passes its format set as the activeFormatSet prop to this component, is also responsible for creating an immutable format set and keeping it up to date with the help of FormatSetFormatsProvider.onFormatsChanged.
There was a problem hiding this comment.
That's a good point, okay I reverted this, and documented the instruction in the README. there's also a doccomment on the activeFormatSet prop to recommend devs to pass a new object after FormatSetFormatsProvider.onFormatsChanged
There was a problem hiding this comment.
I guess one quality-of-life improvement we could do for consumers is to provide a hook that takes care of returning an immutable active format set and making sure it's up to date, based on FormatSetFormatsProvider.onFormatsChanged. What do you think about that?
There was a problem hiding this comment.
I like that - I've added a useFormatSet(formatSet, provider) hook.
It returns a copy that's replaced after each onFormatsChanged. The provider keeps its _formatSet private, so users have to pass the same provider object they created containing that formatSet. I added a doc comment spelling out what happens if you don't.
If core does expose formatSet getter on the provider, the hook could take just the provider... but I would want to make sure that getter returns a clone, not the actual referenced formatSet object.
… publishes a new format set on changes FormatSelector goes back to memoizing by the formats object. Owners of a FormatSetFormatsProvider must pass a new format set after onFormatsChanged, which the README now documents and test-viewer's FormatManager does.
…dead FractionDash handler, scope test hooks
…ead of a FormatManager event useFormatSet(formatSet, formatsProvider) returns a copy of the format set that is replaced after each onFormatsChanged, so FormatSelector shows applied changes when FormatSetFormatsProvider updates the set in place.
TL;DR
Five bugs reported by our users made formats hard or impossible to edit in the quantity formatting UI. Four are fixed in the package. The fifth (#1825) comes from apps changing the format set in place, so
activeFormatSetis now documented as immutable and a newuseFormatSethook gives apps a fresh copy after each change. Regression tests fail on the current code. Minor release because of the hook; no peer dependency changes.Stacked on #1826. Closes #1824, closes #1825.
What was wrong
.and,.FormatSetFormatsProvider.addFormatupdates the format set in place, so the object passed toFormatSelectornever changes and its memoized list keeps the old definitions.activeFormatSetis documented as immutable. NewuseFormatSet(formatSet, provider)returns a copy that is replaced after eachonFormatsChanged; test-viewer uses it, and apps holding aFormatSetFormatsProvidershould too.ShowUnitLabel; the checkboxes only matchedshowUnitLabel. Core reads either, so the label kept showing. Same for every trait checkbox.What
internal/FormatPropsUtils.ts(new)DecimalSeparator/ThousandsSeparatoruseFormatSet(new,@beta)onFormatsChanged. The provider doesn't expose its format set, so callers pass the same object the provider was created with; the doc comment spells out what happens otherwise.FormatSelector/ READMEactiveFormatSetmust be replaced, not changed in place, and point touseFormatSet. Also localizes the "no formats found" message.QuantityFormatButtonuseFormatSet(...)to the selector, so applied changes show up.FormatUnitsLoggerinstead ofconsole.warn.FormatType(format type picker)HORIZONTAL_DIR_*units. Ignores a unit lookup that finishes after a newer change or after the picker unmounts.AppendUnitLabel.tsx/UomSeparatorSelector.tsxFormatUnitLabel.tsx, one component per file.misc/UnitDescrgetUnitNamehelper is kept.useFormatSet.Tests
The regression tests fail against the original source. Control tests, such as the azimuth defaults being applied when nothing changed, pass on both.
New and updated test coverage
TraitCheckboxes.test.tsx— PascalCase trait shown checked / cleared (×6 controls)AppendUnitLabel.test.tsx— PascalCaseShowUnitLabelFormatPropsUtils.test.ts— trait helpersFormatPropsUtils.test.ts— separator conflictsThousandsSeparator.test.tsx— enabling with comma decimalDecimalSeparator.test.tsx— clash / grouping off / locale defaultFormatUnits.test.tsx— degree→minute, minute→second, horizontal-direction degree→minuteFormatUnits.test.tsx— excludes grad, excludes large non-whole ratioFormatUnits.test.tsx— default label placeholderFormatType.test.tsx— azimuth/bearing for horizontal directions and angles45/N45Eend to end.useFormatSet.test.tsxaddFormat, switching format sets, and the stale result when given a different object.FormatType.test.tsx— pending azimuth lookuponChange; one that finishes with nothing changed does.Validation
tsc,pnpm lint,pnpm test(203/203),pnpm build,extract-api(addsuseFormatSet), andcheck-extractionsall pass. test-viewer typechecks and lints clean; it has not been smoke-tested again since the review changes.Nambot 🤖 (powered by claude-opus-5.5)