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

Leverage appropriate number format options in number-text table column once supported in all browsers #1442

Closed
mollykreis opened this issue Aug 18, 2023 · 3 comments · Fixed by #1791
Assignees
Labels
blocked Blocked on a third-party issue tech debt

Comments

@mollykreis
Copy link
Contributor

🧹 Tech Debt

As part of #1414, we had to make some concessions and add some work-arounds due to certain number format options not being supported yet in Firefox.

Once Firefox has parity support with other browsers for these options for at least 6 months we should update our numeric formatters for the number-text table column to use these features.

Specifically the options we would like to use but cannot yet are:

  • useGrouping: auto
  • signDisplay: negative
  • roundingPriority: 'lessPrecision'
@mollykreis mollykreis added tech debt triage New issue that needs to be reviewed labels Aug 18, 2023
@mollykreis mollykreis changed the title Leverage appropriate number format options once supported in all browsers Leverage appropriate number format options in number-text table column once supported in all browsers Aug 18, 2023
@m-akinc m-akinc added blocked Blocked on a third-party issue and removed triage New issue that needs to be reviewed labels Aug 22, 2023
@m-akinc
Copy link
Contributor

m-akinc commented Aug 22, 2023

Should check on this again in early 2024

@m-akinc m-akinc removed the blocked Blocked on a third-party issue label Jan 8, 2024
@m-akinc m-akinc self-assigned this Jan 8, 2024
@rajsite
Copy link
Member

rajsite commented Jan 8, 2024

@m-akinc please wait to address this till after the unit support is merged

@m-akinc
Copy link
Contributor

m-akinc commented Feb 9, 2024

Waiting for a Chromatic update to get new enough browsers to support these options. Could be any time now.

@m-akinc m-akinc added the blocked Blocked on a third-party issue label Feb 9, 2024
rajsite pushed a commit that referenced this issue Feb 26, 2024
# Pull Request

## 🤨 Rationale

Fixes: #1442 

## 👩‍💻 Implementation

The options cited in the issue are:

- `useGrouping: 'auto'` - We previously were setting this to `true`,
which means "display grouping separators even if the locale prefers
otherwise." Instead we want `"auto"`, which is "display grouping
separators based on the locale preference." The default for this option
is `"auto"` unless `notation` is `"compact"`. We never set `notation` to
`"compact"`, so we can just leave `useGrouping` to use the default.
- `signDisplay: 'negative'` - Though this option value has been
[supported in all browsers since
8/23](https://caniuse.com/mdn-javascript_builtins_intl_numberformat_numberformat_options_signdisplay_parameter_negative),
there does not seem to be a version of Typescript whose definition of
`Intl.NumberFormatOptions` allows it. There is an [open issue for adding
support](microsoft/TypeScript#56269). As a
workaround, I've just added type assertions where necessary.
- `roundingPriority: 'lessPrecision'` - Just like `signDisplay:
'negative'`, the `roundingPriority` option has been [supported in
browsers since
8/23](https://caniuse.com/mdn-javascript_builtins_intl_numberformat_numberformat_options_roundingpriority_parameter),
but Typescript doesn't have support yet. It is tracked by the same issue
linked above, and our type assertion serves as a workaround for this,
too. By using this option, we can get rid of the separate
`leadingZeroFormatter` in `DefaultUnitFormat` and simplify the logic.

## 🧪 Testing

Existing tests pass.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or
determined no changes are needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked on a third-party issue tech debt
Projects
Development

Successfully merging a pull request may close this issue.

3 participants