Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Removing spec text that entangles style: "unit" with style: "percent". #57

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

sffc
Copy link
Collaborator

@sffc sffc commented Jul 15, 2019

Fixes #44

@sffc sffc requested review from leobalter and FrankYFTang July 15, 2019 21:53
@FrankYFTang
Copy link
Contributor

how will this change observable behavior?

@sffc
Copy link
Collaborator Author

sffc commented Jul 15, 2019

how will this change observable behavior?

You will no longer be able to combine style: "percent" with unitWidth:

new Intl.NumberFormat(undefined, {
  style: "percent",
  unitDisplay: "long"  // Read, but ignored.
});

new Intl.NumberFormat(undefined, {
  style: "unit",
  unit: "percent",
  unitDisplay: "long"  // This is OK
});

There should be no unit or unitDisplay field in resolvedOptions:

new Intl.NumberFormat(undefined, {
  style: "percent"
}).resolvedOptions();
// should NOT contain a unit or unitDisplay field

percentSign is no longer allowed in formatToParts in style: "unit", but it is still OK in style: "percent":

new Intl.NumberFormat(undefined, {
  style: "unit",
  unit: "percent"
}).formatToParts(100);

// Expected: [
//   { type: "integer", value: "100" },
//   { type: "unit", value: "%" }
// ]

@FrankYFTang
Copy link
Contributor

how will this change observable behavior?

You will no longer be able to combine style: "percent" with unitWidth:

new Intl.NumberFormat(undefined, {
  style: "percent",
  unitWidth: "long"  // Read, but ignored.
});

What is "unitWidth"? Do you mean "unitDisplay"?

new Intl.NumberFormat(undefined, {
style: "unit",
unit: "percent",
unitWidth: "long" // This is OK
});

There should be no unit or unitWidth field in resolvedOptions:

new Intl.NumberFormat(undefined, {
  style: "percent"
}).resolvedOptions();
// should NOT contain a unit or unitWidth field

What is "unitWidth"? Do you mean "unitDisplay"?

@sffc
Copy link
Collaborator Author

sffc commented Jul 16, 2019

What is "unitWidth"? Do you mean "unitDisplay"?

Yes, sorry, my bad. Getting the ICU name mixed up with the 402 name. I'll edit my post

Copy link
Contributor

@FrankYFTang FrankYFTang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sffc sffc merged commit cb56ac2 into master Jul 16, 2019
@sffc sffc deleted the percent branch July 16, 2019 23:58
pull bot pushed a commit to Richienb/v8 that referenced this pull request Jul 19, 2019
1. Sync with
tc39/proposal-unified-intl-numberformat#57
so the formatting of {style: "unit" unit: "percent"} and
the formatting of {style: "percent:"} are treated different that
simplified the algorithm.
2. Store style into bit flags because we need it quickly during format.
3. Add more unit tests and regression test.

Bug: v8:9498
Change-Id: I75ed22fef1feb73ebf624bda70ebe45b80e7bc8b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1704948
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Frank Tang <ftang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62834}
@sffc sffc changed the title Removing spec text that entangles style: "unit" with style: "percent". NORMATIVE: Removing spec text that entangles style: "unit" with style: "percent". Dec 19, 2019
@sffc sffc changed the title NORMATIVE: Removing spec text that entangles style: "unit" with style: "percent". Removing spec text that entangles style: "unit" with style: "percent". Dec 19, 2019
@sffc sffc added the NORMATIVE PR contains a behavior change label Dec 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NORMATIVE PR contains a behavior change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

style: 'percent' vs {style: 'unit' unit: 'percent'}
2 participants