chore: move to Intl.NumberFormat for formatting numbers#120
chore: move to Intl.NumberFormat for formatting numbers#120Pixselve wants to merge 5 commits intocloudscape-design:mainfrom
Conversation
| if (absValue >= 1e3) { | ||
| return format(value / 1e3) + "K"; | ||
| // Use scientific notation for very small numbers | ||
| if (absValue < 1e-9) { |
There was a problem hiding this comment.
Is there a specific reason why the threshold of this condition changes from 0.01 (i.e, 1e-2) to 1e-9?
@pan-kot Would this be a problem?
There was a problem hiding this comment.
In this PR, I also updated how numbers are truncated, to reflect how CloudWatch charts are currently behaving. The number
0.00086661402is truncated in CDS, but not on the console.
I am open to the discussion about that. It may be valuable to expose that setting to the chart props?
There was a problem hiding this comment.
You can do that in the test pages - but let's not change the current precision in the default chart formatters. What is good for one team might not work for another.
All formatters can be already overridden in both public and core APIs.
| expect(formatter.call(mockAxisContext({ value: 0.03 }))).toBe("0.03"); | ||
| expect(formatter.call(mockAxisContext({ value: 0.003 }))).toBe("3e-3"); | ||
| expect(formatter.call(mockAxisContext({ value: 0.003 }))).toBe("0.003"); | ||
| expect(formatter.call(mockAxisContext({ value: 0.0000000003 }))).toBe("3E-10"); |
There was a problem hiding this comment.
It is based on the Unicode Common Locale Data Repository. See https://cldr.unicode.org/translation/number-currency-formats/number-and-currency-patterns
| Type | Example pattern | Meaning |
|---|---|---|
| E | # E 0 |
This marks a scientific format. The E symbol may change position. Must be retained. |
|
Fixed the functional test |
| "jsx": "react-jsx", | ||
| "types": [], | ||
| "lib": ["es2019", "dom", "dom.iterable"], | ||
| "lib": ["es2020", "dom", "dom.iterable"], |
| const chart = w.findCartesianHighcharts('[data-testid="grouped-column-chart"]'); | ||
| const point = chart.find('[aria-label="Jul 2019 6.32K, Prev costs"]'); | ||
| const expectedTooltipContent = ["Jul 2019\nCosts\n8.77K\nPrev costs\n6.32K"]; | ||
| const point = chart.find('[aria-label="Jul 2019 6.322K, Prev costs"]'); |
There was a problem hiding this comment.
why did we increase the default precision?
src/core/formatters.tsx
Outdated
| function numberFormatter(value: number): string { | ||
| const format = (num: number) => parseFloat(num.toFixed(2)).toString(); // trims unnecessary decimals | ||
|
|
||
| function numberFormatter(value: number, locale: string = "en-US"): string { |
There was a problem hiding this comment.
should we take locale from the document instead?
There was a problem hiding this comment.
Made a PR to share the locale getting logic: cloudscape-design/component-toolkit#176
| test("uses default numeric axes formatters for integer values", () => { | ||
| renderChart({ highcharts, options: { series, xAxis: { title: { text: "X" } }, yAxis: { title: { text: "Y" } } } }); | ||
| getAxisOptionsFormatters().forEach((formatter) => { | ||
| // See https://www.unicode.org/cldr/cldr-aux/charts/29/verify/numbers/en.html |
There was a problem hiding this comment.
let's better add this reference to the formatter function, not tests
# Conflicts: # src/core/chart-core.tsx # src/core/components/core-tooltip.tsx
|
Depends on cloudscape-design/component-toolkit#180 |
3 participants
Description
This PR move the number formatting to using the native helper
Intl.NumberFormat.What is Intl.NumberFormat
See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat
It is a set of helpers to format number based on the provided
locale.Intl.NumberFormatis widely available in all browsers since 2017. Options supports varies.✅
notation✅
maximumFractionDigits✅
compactDisplayWhy
1,000,000,000should be1B(instead of1Glike in the code) (see https://www.unicode.org/cldr/cldr-aux/charts/29/verify/numbers/en.html)locale(currently hardcoded toen-US) to format numbers based on the user's language.In this PR, I also updated how numbers are truncated, to reflect how CloudWatch charts are currently behaving. The number
0.00086661402is truncated in CDS, but not on the console.How has this been tested?
Review checklist
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.