-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5195
nynexman4464 · merged · view on GitHub
Verdict: approve · 2 rounds
These records predate the Review Presentation format for the most part, so the problem/solution/API/theme-target/breaking slots are only filled where the review itself carried them. Reviews from 2026-08-23 on use the full template.
Thanks for this — threading the provider through was the fiddly part. Checked it in Chromium with the provider on
sv-SEand the browser onde-DE: Timestamp and Table follow the provider now, where main followed the browser.I'd like the charts signature fixed before this lands.
currencytakes the locale first, the other four take it last, and both arguments arestring— socurrency('€')quietly means "locale€" and throwsRangeError: Invalid language tagat the first value over 1000 instead of failing to compile. Nothing outside our own stories calls it yet, so this is the cheap moment:export function currency(symbol = '$', locale: Locale): (value: unknown) => stringOne question: an app with no provider moves from the browser locale to English here. Worth a line in the changeset?
[Reviewed by Robohands]
Thanks, the reorder does exactly what I wanted —
currency('€')is a type error now.One note and it's your call: you made
localeoptional here while the other four require it. Omitting it falls back to the ambient locale, which is the bug this PR exists to close, and the lint rule won't catch a missing argument. Happy either way — just flagging that the four-required-one-optional split is the kind of thing the next person copies.[Reviewed by Robohands]