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

fractionalDigits, (||milli|micro|nano)secondsDisplay impact to number of fractional digits while style: "digital" #139

Closed
FrankYFTang opened this issue Mar 22, 2023 · 9 comments
Labels
consensus We reached a consensus in a discussion meeting, through email or the issue discussion normative

Comments

@FrankYFTang
Copy link
Collaborator

FrankYFTang commented Mar 22, 2023

So... we have the following options in the object creation all seems connect to the formating result of the "second". While the style is not "digital", these options are not interacted with each other so it is easy to understand, however, I am not sure what is the current intended result when the style is "digital"

fractionalDigits: 0...9 (default 0)
secondsDisplay: 'auto' | 'always' (default auto)
millisecondsDisplay: 'auto' | 'always' (default auto)
microsecondsDisplay: 'auto' | 'always' (default auto)
nanosecondsDisplay: 'auto' | 'always' (default auto)

consider the following data

let d1 = {minutes: 9, seconds: 1};
let d2 = {minutes: 9, milliseconds: 2};
let d3 = {minutes: 9, seconds: 1, milliseconds: 2};
let d4 = {minutes: 9, microseconds: 3};
let d5 = {minutes: 9, seconds: 1, miicroseconds: 3};
let d6 = {minutes: 9, nanoseconds: 4};
let d7 = {minutes: 9, seconds: 1, miicroseconds: 4};

Looking from the current spec, it seems the only option decide the number of formatted fractional digits is solely controlled by fractionalDigits and the output is not impacted by the present of secondsDisplay, millisecondsDisplay, microsecondsDisplay or nanosecondsDisplay .
For example, if we have the following code

(new Intl.DurationFormat('en', {style: 'digital', millisecondsDisplay: 'always' })).format(d3)

it seems the current spec text will output "9:01" instead of "9:01.003"

but is that what we like to see?
If we will ignore millisecondsDisplay, microsecondsDisplay and nanosecondsDisplay while the style is "digital", then should we throw in such condition during the constrcutor ?

If we will not ignore them, then what should we do if we also have the presence of fractionalDigits ?

@ryzokuken @sffc

@FrankYFTang FrankYFTang changed the title In style: "digital" how is fractionalDigits, secondsDisplay, millisecondsStyle, microsecondsStyle, and nanosecondsStyle impact the number of fractional digits in the result string fractionalDigits, (||milli|micro|nano)secondsStyle impact to number of fractional digits while style: "digital" Mar 22, 2023
@sffc
Copy link
Collaborator

sffc commented Mar 31, 2023

It's a bit late to make changes, but reasonable behavior could be to change the default millisecondStyle, microsecondStyle, and nanosecondStyle in style: "digital" to be "short" if the Display for that unit is "always".

@FrankYFTang
Copy link
Collaborator Author

I think you didn't get my point. the issue is with style: "digital" (and some other cases that the fractional second units fold to be a higher unit because it is set to 'numeric'), any units below second is now part of second and never displayed by itself. (so the formatToParts will not have a unit with these unit when the style is digital) so how would the display for these units make sense when it is "always"? (since it cannot be "always" in that case)

@sffc
Copy link
Collaborator

sffc commented Apr 1, 2023

I thought we were allowing { style: "digital", millisecondsStyle: "short" }, producing output like "0:00:00, 0 ms" but maybe we don't actually allow that.

@FrankYFTang
Copy link
Collaborator Author

I thought we were allowing { style: "digital", millisecondsStyle: "short" }, producing output like "0:00:00, 0 ms" but maybe we don't actually allow that.
do you mean

{ style: "digital", milliseconds: "short" } ?

@FrankYFTang
Copy link
Collaborator Author

FrankYFTang commented Apr 3, 2023

If we have { style: "digital", milliseconds: "short" } in the option, step 6-a-i of GetDurationUnitOptions will throw RangeError now because

under the iterator of the table, when the GetDurationUnitOptions got called with "seconds" as unit, the return style will be set to "numeric" by step 3-a-ii
"ii. Set style to digitalBase."
so the next iteration GetDurationUnitOptions will come with unit as "milliseconds", prevStyle: "numeric"
and then in step 1

  1. Let style be ? GetOption(options, unit, "string", stylesList, undefined).
    will now get "short" as styel but then in step 6
  2. If prevStyle is "numeric" or "2-digit", then
    it will be true
    and
    6-a. If style is not "numeric" or "2-digit", then
    is also trye (since we now have style: "short"
    and in 6-a-i we will i. Throw a RangeError exception.

@ryzokuken
Copy link
Member

@FrankYFTang is correct, you cannot tweak the milliseconds style option to something that doesn't work for the overall style "digital" and in this situation, the display options aren't perfectly respected since they make most sense for non-digital styles too. This returns us to the general theme of digital being a difficult style to design around and trying to accommodate the style alongside the options for the non-digital style will always have some drawbacks, we just need to find out which of them we can live with.

One option I feel works best is to allow only one of the *secondsDisplay options or fractionalDigits for the digital style, since they counteract each other, or perhaps even going farther and allowing only fractionalDigits to be used for the digital style, disallowing setting *secondsDisplay to "always" completely for that style since it applies a different display strategy for these units anyway.

@FrankYFTang
Copy link
Collaborator Author

One option I feel works best is to allow only one of the *secondsDisplay options or fractionalDigits for the digital style,
the issue is .+secondsDisplay: 'always' (not *secondsDisplay) is conflicting with style: 'digital' because under digital, we never output any units under 'milliseconds", "microseconds", or "nanoseconds" in the formatToParts unit, right?

@ryzokuken
Copy link
Member

the issue is .+secondsDisplay: 'always' (not *secondsDisplay) is conflicting with style: 'digital' because under digital, we never output any units under 'milliseconds", "microseconds", or "nanoseconds" in the formatToParts unit, right?

you're right, syntax error from my side but basically this. Anything below seconds will be handled as a fraction in "digital" which means the +display doesn't make sense in that case.

@FrankYFTang FrankYFTang changed the title fractionalDigits, (||milli|micro|nano)secondsStyle impact to number of fractional digits while style: "digital" fractionalDigits, (||milli|micro|nano)secondsDisplay impact to number of fractional digits while style: "digital" Apr 6, 2023
@sffc sffc added consensus We reached a consensus in a discussion meeting, through email or the issue discussion and removed Meeting Discussion Need to be discussed in one of the upcoming meetings labels Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus We reached a consensus in a discussion meeting, through email or the issue discussion normative
Projects
None yet
Development

No branches or pull requests

3 participants