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

DateTimeFormat - restructure constructor parameters #11544

Merged
merged 11 commits into from
Jul 22, 2021

Conversation

hamishwillee
Copy link
Contributor

This follows on from #11501

This is just the first step - restructure the constructor to use subfeatures for parameters, and subfeatures of those for options.

I am in no way sure this is "correct" so this is a first step.

Following on from this I will need to add the new values for timeZoneName added in FF91 - see https://bugzilla.mozilla.org/show_bug.cgi?id=1710429.

@github-actions github-actions bot added the data:js Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript label Jul 16, 2021
@hamishwillee
Copy link
Contributor Author

The next step is to add options_timeZoneName_parameter.

  1. My inference is that I create options_timeZoneName_parameter with a subfeature options_timeZoneName_parameter_ana to mirror the timeZone care - right?
  2. What do I put for the versions in options_timeZoneName_parameter - do I just assume this option was introduced at the earliest versions of the constructor?
  3. Can you help me work out what version of v8 the new options went into?

@Elchi3
Copy link
Member

Elchi3 commented Jul 19, 2021

  • My inference is that I create options_timeZoneName_parameter with a subfeature options_timeZoneName_parameter_ana to mirror the timeZone care - right?

This works, I wonder if we're nesting too deep at this point. I think it wouldn't hurt to have it at the same level. I have no strong feeling.

  • What do I put for the versions in options_timeZoneName_parameter - do I just assume this option was introduced at the earliest versions of the constructor?

I think that makes sense, yes.

  • Can you help me work out what version of v8 the new options went into?

I can't unfortunately.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Given options is an object, a dot feels better to me.

javascript/builtins/intl/DateTimeFormat.json Outdated Show resolved Hide resolved
javascript/builtins/intl/DateTimeFormat.json Outdated Show resolved Hide resolved
javascript/builtins/intl/DateTimeFormat.json Outdated Show resolved Hide resolved
javascript/builtins/intl/DateTimeFormat.json Outdated Show resolved Hide resolved
javascript/builtins/intl/DateTimeFormat.json Outdated Show resolved Hide resolved
javascript/builtins/intl/DateTimeFormat.json Outdated Show resolved Hide resolved
javascript/builtins/intl/DateTimeFormat.json Outdated Show resolved Hide resolved
@hamishwillee hamishwillee requested a review from Elchi3 July 20, 2021 07:32
@hamishwillee
Copy link
Contributor Author

@Elchi3 Thanks for your review and advice: #11544 (comment)

I have merged your suggestions, and also updated the timeZone versions to match the constructor, where they didn't already.

Lastly I have added the timeZoneName option and iana subfeatures, mirroring those for timeZone. That makes sense because whether or not it is too deeply nested, we should be consistent. All other browsers are marked false for the new feature - though I only tested on latest Chrome.

I think ready for rereview.

Copy link
Member

@Elchi3 Elchi3 left a comment

Choose a reason for hiding this comment

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

Thanks Hamish! 👍

@Elchi3 Elchi3 merged commit e39287a into mdn:main Jul 22, 2021
@ddbeck
Copy link
Collaborator

ddbeck commented Jul 22, 2021

This PR affected the following features:

Removed

  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.dateStyle
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.dayPeriod
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.fractionalSecondDigits
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.hourCycle
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.iana_time_zone_names
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.timeStyle

Added

  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.locales_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_dateStyle_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_dayPeriod_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_fractionalSecondDigits_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_hourCycle_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_timeStyle_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_timeZone_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_timeZone_parameter.options_timeZone_parameter_iana
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_timeZoneName_parameter
  • javascript.builtins.Intl.DateTimeFormat.DateTimeFormat.options_parameter.options_timeZoneName_parameter.options_timeZoneName_parameter_iana

ddbeck added a commit to ddbeck/browser-compat-data that referenced this pull request Jul 22, 2021
ddbeck added a commit that referenced this pull request Jul 22, 2021
* Bump version to v3.3.12

* Add release note for #11661

* Add release notes for
#11173 and #11175

* Add release note for #11534

* Add release note for #11544

* Add release note for #11551

* Add release note for #11555

* Add release note for #11556

* Add release note for #11557

* Add release note for #11633

* Add release note for #11636

* Add release note for #11637

* Add release note for #11530

* Add release stats and date

* Consolidate Safari for iOS version consolidation notes

* Format PR URL
@hamishwillee hamishwillee deleted the ff81_date_iana_options branch July 22, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:js Compat data for JS/ECMAScript features. https://developer.mozilla.org/docs/Web/JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants