-
Notifications
You must be signed in to change notification settings - Fork 14
Review for changes to specification for Unified NumberFormat #8
Conversation
…t on the number being formatted.
… currencyDisplay enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some suggestions on the explainer; I'll come back and review the spec text a bit later.
README.md
Outdated
|
||
There are many requests for adding number-formatting-related features to ECMA 402. A few of them include: | ||
There are many requests for adding number-formatting-related features to ECMA 402. These include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For background, write "ECMA-402 (the JavaScript Intl standard library)" to make this more self-contained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specification looks generally good to me. The organization seems clear, and I don't have any concerns about the semantics that you're exporting. I just have some editorial comments. Thanks for your patience here!
numberformat_proposed.html
Outdated
<li>[[Currency]] is a String value with the currency code identifying the currency to be used if formatting with the `"currency"` style. It is only used when [[Style]] has the value `"currency"`.</li> | ||
<li>[[CurrencyDisplay]] is one of the String values `"code"`, `"symbol"`, or `"name"`, specifying whether to display the currency as an ISO 4217 alphabetic currency code, a localized currency symbol, or a localized currency name if formatting with the `"currency"` style. It is only used when [[Style]] has the value `"currency"`.</li> | ||
<li>[[Style]] is one of the String values `"decimal"`, `"currency"`, `"percent"`, or `"unit"`. This value corresponds closely with the [[UnitType]] value.</li> | ||
<li>[[UnitType]] is one of the String values `"none"`, `"currency"`, or `"measure-unit"`. The value `"measure-unit"` is used for both [[Style]] `"percent"` and `"unit"`.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is UnitType represented separately from Style? It seems like the only difference here is organizing "percent" as a kind of "measure-unit"--am I misunderstanding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style is public; UnitType is internal. UnitType maps more directly to data and ICU.
At some point I need to make the resolution from percent to measure-unit; it seems cleanest to do that at the API boundary and store the result in UnitType.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I can see how UnitType corresponds to what you have in your locale database, and Style corresponds to the options bag and resolvedOptions, but it still seems pretty redundant to me. I don't have an opinion whether it's stored in the object as Style or UnitType, but I'd prefer logic at the usage sites to translate from one to the other rather than redundantly storing the information.
…rrency and measurement units.
… new Record schema.
Add currency sign to accounting example
Change unit example to common speed quantity.
Fixes #49
Change all kebab case to camel case in numberformat Section 11.
Compact rounding for small numbers should retain 2 significant digits.
Removing spec text that entangles style: "unit" with style: "percent".
Change unit list to a Table, and fix casing of Record
Thanks for the updates here. It seems like in good shape as well for now. TBH, sounds easier for me to review everything from a single branch rather than as in any patch. |
This is the original PR that was used for Stage 3 review. Any further changes from Stage 3 have been separate PRs. I'll close this PR to reduce confusion. |
No description provided.