fixup explainer and update to latest design #66

ryzokuken · 2021-08-04T09:43:29Z

update explainer
update spec

/cc @justingrant @sffc @FrankYFTang

README.md

justingrant · 2021-08-04T18:36:20Z

README.md

 ```
+


There should be a section explaining the motivation and design for the fractionalDigits option. Also a section for the style option.

Finally, I was hoping to see an "Options Summary" or "Design Summary" section that shows the full list of options and their allowed values. The current layout of the page with multiple, separate "Design" sections is fine, but it makes it visualize every option to understand the big picture of how the API works. I think both are helpful.

The new fractionalDigits section with examples is great. But IMHO this page still needs an options summary section which lists all the possible options, their possible values, and a short sentence about each one. Something like the "parameters" section of https://tc39.es/proposal-temporal/docs/zoneddatetime.html#until.

@justingrant updated! Please check out the README now and let me know what you think.

New content is much clearer and easier to understand. Thanks!

justingrant · 2021-08-04T18:44:37Z

spec.emu

+      <li>[[MonthsDisplay]] is one of the String values `"auto"` or `"always"` identifying when to display the months field.</li>
+      <li>[[Weeks]] is one of the String values `"long"`, `"short"`, or `"narrow"` identifying the formatting style used for the weeks field.</li>
+      <li>[[WeeksDisplay]] is one of the String values `"auto"` or `"always"` identifying when to display the weeks field.</li>
+      <li>[[Days]] is one of the String values `"long"`, `"short"`, or `"narrow"` identifying the formatting style used for the days field.</li>


I think we should consider supporting a 'numeric' style for days. I've seen countdown timers that add days to the left of hours, e.g. 2:23:59:59.123456789.

justingrant · 2021-08-04T18:45:16Z

spec.emu

@@ -331,7 +405,26 @@ contributors: Ujjwal Sharma, Younies Mahmoud
      <li>[[DataLocale]] is a String value with the language tag of the nearest locale for which the implementation has data to perform the formatting operation. It will be a parent locale of [[Locale]].</li>
      <li>[[NumberingSystem]] is a String value with the `"type"` given in Unicode Technical Standard 35 for the numbering system used for formatting.</li>
      <li>[[Style]] is one of the String values `"long"`, `"short"`, `"narrow"`, or `"digital"` identifying the duration formatting style used.</li>
-      <li>[[Fields]] is a List value specifying which duration fields are required to be printed.</li>
+      <li>[[Years]] is one of the String values `"long"`, `"short"`, or `"narrow"` identifying the formatting style used for the years field.</li>


Where is fractionalDigits option?

README.md

ryzokuken · 2021-08-05T14:44:05Z

Thanks @sffc and @justingrant.

ryzokuken · 2021-08-05T16:22:20Z

@justingrant does this note seem good?

justingrant · 2021-08-06T04:53:34Z

does this note seem good?

Text for fractionalDigits is OK, but I'd suggest adding a few examples to show how it works. It's a pretty confusing option to explain with only text. Also, may want to link to #64 and/or other open issues around this option.

ryzokuken · 2021-08-13T11:21:07Z

@justingrant this should do it, right? Seems like the explainer is good to go, would you mind if I extracted the spec commits from this and merged the explainer?

README.md

justingrant · 2021-08-13T11:57:49Z

Seems like the explainer is good to go, would you mind if I extracted the spec commits from this and merged the explainer?

I'd prefer to see an options summary section added (see #66 (comment)) before merging the new explainer. I think removing the spec updates to a separate PR makes sense to iterate on both in parallel.

ryzokuken · 2021-08-17T12:43:04Z

@justingrant could you check it out now and let me know how it looks?

justingrant

I really like the new reference section, because it helps highlight some possible disconnects. Lots of comments below!

README.md

justingrant · 2021-08-17T14:23:42Z

README.md

+  * `minutesDisplay` (`String`): When to display minutes. `"auto"` means that minutes won't be displayed if they are zero. Can be either `"always"` or `"auto"`. Defaults to `"auto"` if `minutes` is `undefined` and `"always"` otherwise.
+  * `seconds` (`String`): The style to be used for formatting seconds. Can be one of `"long"`, `"short"`, or `"narrow"`. Defaults to the value of `style`.
+  * `secondsDisplay` (`String`): When to display seconds. `"auto"` means that seconds won't be displayed if they are zero. Can be either `"always"` or `"auto"`. Defaults to `"auto"` if `seconds` is `undefined` and `"always"` otherwise.
+  * `milliseconds` (`String`): The style to be used for formatting milliseconds. Can be one of `"long"`, `"short"`, or `"narrow"`. Defaults to the value of `style`.


I'd expect some discussion here (and in the microseconds and nanoseconds docs below) about:

when ms/μs/ns values will be displayed as decimals after seconds (or after milliseconds or microseconds for smaller units) and when they'll be displayed as standalone values like "123 ms".

how fractionalDigits affects display of ms/μs/ns units

whether trailing zeroes are shown or hidden by default

README.md

justingrant

New content looks really good. Makes it much easier to understand, and also easy to highlight a small number of things that I commented on. I didn't review the spec yet but can do that once the explainer content is revised.

README.md

justingrant · 2021-08-20T05:04:53Z

README.md

 ```
+


New content is much clearer and easier to understand. Thanks!

README.md

justingrant

Explainer text looks great, modulo a few minor comments I added. I moved on to review the spec and it looks like a few things are missing there, e.g. fractionalDigits, the numeric->2-digit upgrade, defaults for digital style (maybe), etc.

README.md

justingrant · 2021-08-20T18:36:30Z

spec.emu

@@ -331,7 +445,26 @@ contributors: Ujjwal Sharma, Younies Mahmoud
      <li>[[DataLocale]] is a String value with the language tag of the nearest locale for which the implementation has data to perform the formatting operation. It will be a parent locale of [[Locale]].</li>
      <li>[[NumberingSystem]] is a String value with the `"type"` given in Unicode Technical Standard 35 for the numbering system used for formatting.</li>
      <li>[[Style]] is one of the String values `"long"`, `"short"`, `"narrow"`, or `"digital"` identifying the duration formatting style used.</li>
-      <li>[[Fields]] is a List value specifying which duration fields are required to be printed.</li>
+      <li>[[YearsStyle]] is one of the String values `"long"`, `"short"`, or `"narrow"` identifying the formatting style used for the years field.</li>


where is fractionalDigits?

justingrant · 2021-08-20T18:38:18Z

spec.emu

+            1. Set _stylesList_ to the list-concatenation of _stylesList_ and &laquo; "2-digit" &raquo;.
+        1. Let _style_ be ? GetOption(_options_, _unit_, _stylesList_, *undefined*).
+        1. If _style_ is *undefined*, then
+          1. Set _style_ to _baseStyle_.


What if baseStyle is digital?

justingrant · 2021-08-20T18:40:53Z

spec.emu

+
+      <emu-alg>
+        1. Let _stylesList_ be the List &laquo; `"long"`, `"short"`, `"narrow"` &raquo;.
+        1. If _unit_ is one of `"hours"`, `"minutes"`, `"seconds"`, `"milliseconds"`, `"microseconds"`, or `"nanoseconds"`, then


where does the upgrade from numeric to 2-digit happen?

justingrant · 2021-08-20T18:43:08Z

spec.emu

+      <p>The GetUnitOptions abstract operation is called with the arguments _unit_ (which must be a String), _options_ (which must be an Object) and _baseStyle_ (which must be a String) and returns a Record containing the relevant options for that unit. The following steps are taken:</p>
+
+      <emu-alg>
+        1. Let _stylesList_ be the List &laquo; `"long"`, `"short"`, `"narrow"` &raquo;.


What happens with weird styles, e.g. {hours: numeric, minutes: 'long'}. Error? Does "long" get upgraded to 2-digit? Something else?

ryzokuken · 2021-08-23T19:08:55Z

@justingrant do you think it's okay to merge this if I take the spec changes out and file a different PR for them?

justingrant · 2021-08-23T19:18:10Z

@justingrant do you think it's okay to merge this if I take the spec changes out and file a different PR for them?

I'm OK with splitting into different PRs, but there are still two outstanding comments on the explainer content that probably should be addressed before merging.

ryzokuken · 2021-08-23T19:36:32Z

@justingrant does the explainer look good to go now?

ryzokuken · 2021-08-24T18:01:20Z

@justingrant can I merge this now? Thanks.

justingrant

Looks good! Thanks for doing all the explainer fixes, we caught and resolved some useful things.

ryzokuken requested review from sffc and FrankYFTang August 4, 2021 09:43

ryzokuken self-assigned this Aug 4, 2021

ryzokuken force-pushed the update-2 branch 2 times, most recently from 697062b to b941a11 Compare August 4, 2021 10:09

ryzokuken mentioned this pull request Aug 4, 2021

spec: update spec text with proposed semantics #62

Closed

ptomato reviewed Aug 4, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

justingrant suggested changes Aug 4, 2021

View reviewed changes

sffc reviewed Aug 4, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

ryzokuken force-pushed the update-2 branch from 338af54 to 70519e0 Compare August 5, 2021 15:50

justingrant reviewed Aug 13, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

justingrant suggested changes Aug 17, 2021

View reviewed changes

sffc reviewed Aug 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

sffc approved these changes Aug 19, 2021

View reviewed changes

justingrant suggested changes Aug 20, 2021

View reviewed changes

ryzokuken force-pushed the update-2 branch from 692197a to 0a26c18 Compare August 23, 2021 19:41

update explainer to latest consensus

e421324

ryzokuken force-pushed the update-2 branch from 04931f5 to e421324 Compare August 23, 2021 19:46

fixup: [spec] npm run build

b4ac59a

justingrant mentioned this pull request Aug 23, 2021

How to handle {hours: 'numeric', minutes: 'long'} ? #69

Closed

justingrant approved these changes Aug 25, 2021

View reviewed changes

ryzokuken merged commit 84aec32 into tc39:master Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixup explainer and update to latest design #66

fixup explainer and update to latest design #66

ryzokuken commented Aug 4, 2021 •

edited

Loading

justingrant Aug 4, 2021

justingrant Aug 13, 2021

ryzokuken Aug 17, 2021

justingrant Aug 20, 2021

justingrant Aug 4, 2021

justingrant Aug 4, 2021

ryzokuken commented Aug 5, 2021

ryzokuken commented Aug 5, 2021

justingrant commented Aug 6, 2021

ryzokuken commented Aug 13, 2021

justingrant commented Aug 13, 2021

ryzokuken commented Aug 17, 2021

justingrant left a comment

justingrant Aug 17, 2021

justingrant left a comment

justingrant Aug 20, 2021

justingrant left a comment

justingrant Aug 20, 2021

justingrant Aug 20, 2021

justingrant Aug 20, 2021

justingrant Aug 20, 2021

ryzokuken commented Aug 23, 2021

justingrant commented Aug 23, 2021

ryzokuken commented Aug 23, 2021

ryzokuken commented Aug 24, 2021

justingrant left a comment

fixup explainer and update to latest design #66

fixup explainer and update to latest design #66

Conversation

ryzokuken commented Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Aug 5, 2021

ryzokuken commented Aug 5, 2021

justingrant commented Aug 6, 2021

ryzokuken commented Aug 13, 2021

justingrant commented Aug 13, 2021

ryzokuken commented Aug 17, 2021

justingrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justingrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justingrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Aug 23, 2021

justingrant commented Aug 23, 2021

ryzokuken commented Aug 23, 2021

ryzokuken commented Aug 24, 2021

justingrant left a comment

Choose a reason for hiding this comment

ryzokuken commented Aug 4, 2021 •

edited

Loading