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

Date: Add formatDateToParts #700

Merged
merged 3 commits into from Apr 12, 2017
Merged

Conversation

rxaviers
Copy link
Member

@rxaviers rxaviers commented Mar 3, 2017

Supersedes #697, Fixes #678

List of part types, its respective CLDR date fields, and comparison with Intl DateTimeFormat formatToParts.

Globalize CLDR date fields Intl formatToParts
era era era
year year year
quarter quarter -
month month month
week week -
day day day
weekday weekday weekday
dayperiod dayperiod dayPeriod
hour hour hour
minute minute minute
second second second
zone zone timeZoneName
literal literal literal

List of part types and the corresponding CLDR date field patterns.

part type CLDR date field CLDR date field patterns
era era G
year year yY
quarter quarter qQ
month month ML
week week wW
day day dDF
weekday weekday ecE
dayperiod dayperiod a
hour hour hHkK
minute minute m
second second sSA
zone zone zOxX
literal literal

Checklist:
Checklist

  • Implementation
    • Functionality
    • Coding style (build passes)
    • formatDateToParts and dateToPartsFormatter.
  • Unit tests
    • Update test/unit/date/format.js
    • Update test/unit/date/format-properties.js
  • Functional tests
    • Update test/functional/date/format-date.js
    • move from test/functional/date/format-date.js into formatDateToParts and dateToPartsFormatter.
  • Runtime code
    • Update src/date-runtime.js
  • Documentation
    • Update doc/api/date/date-to-parts-formatter.md
  • Examples

@jsf-clabot
Copy link

jsf-clabot commented Mar 14, 2017

CLA assistant check
All committers have signed the CLA.

@rxaviers rxaviers added this to the 1.3.0 milestone Mar 17, 2017
@rxaviers rxaviers force-pushed the date-to-parts-2 branch 2 times, most recently from ff349bd to c11b38a Compare Apr 11, 2017
Copy link
Contributor

@jzaefferer jzaefferer left a comment

I started reviewing, then got dragged into a meeting. Didn't get very far, but hopefully the existing comments will help.

@@ -89,7 +89,7 @@ var enFormatter = Globalize( "en" ).dateFormatter(),
deFormatter = Globalize( "de" ).dateFormatter();

enFormatter( new Date( 2010, 10, 30, 17, 55 ) );
// > "11/30/2010, 5:55 PM"
// > "11/30/2010"
Copy link
Contributor

@jzaefferer jzaefferer Apr 11, 2017

Choose a reason for hiding this comment

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

Should the line above still pass the time to the Date constructor? Probably applies to various other examples in this file (and others?) as well.

Copy link
Member Author

@rxaviers rxaviers Apr 11, 2017

Choose a reason for hiding this comment

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

The time part of the Date constructor is irrelevant and could be removed. 👍

Copy link
Member Author

@rxaviers rxaviers Apr 12, 2017

Choose a reason for hiding this comment

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


### Example

Prior to using any date methods, you must load
Copy link
Contributor

@jzaefferer jzaefferer Apr 11, 2017

Choose a reason for hiding this comment

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

Do all docs have this note in the example section? Seems like important information that could be pulled up.

Copy link
Member Author

@rxaviers rxaviers Apr 11, 2017

Choose a reason for hiding this comment

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

Yeap, other modules/fns have the same statement.

formatter( new Date( 2010, 10, 30, 17, 55 ) ).map(({type, value}) => {
switch ( type ) {
case "year": return `<strong>${value}</strong>`;
default : return value;
Copy link
Contributor

@jzaefferer jzaefferer Apr 11, 2017

Choose a reason for hiding this comment

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

misplaced space in front of the colon

Copy link
Member Author

@rxaviers rxaviers Apr 12, 2017

Choose a reason for hiding this comment

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

Thx a9bc0b5


- `day`

The string used for the day, e.g., `"17"`, `"١٦"`.
Copy link
Contributor

@jzaefferer jzaefferer Apr 11, 2017

Choose a reason for hiding this comment

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

Could compress this section a lot by putting the type of description on one line.

- `day`: The string used for the day, e.g., `"17"`, `"١٦"`.

Copy link
Member Author

@rxaviers rxaviers Apr 11, 2017

Choose a reason for hiding this comment

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

Yeap, I think it needs overall improvement. I dislike the current format of the API, I am wondering if we had something like https://github.com/rxaviers/relative-time#formatdate-options instead.

Copy link
Member Author

@rxaviers rxaviers Apr 12, 2017

Choose a reason for hiding this comment

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

Let's track overall docs improvement in a separate PR.

Globalize.locale( "en" );
formatter = Globalize.dateToPartsFormatter();

formatter( new Date( 2010, 10, 30, 17, 55 ) ).map(({type, value}) => {
Copy link
Contributor

@jzaefferer jzaefferer Apr 11, 2017

Choose a reason for hiding this comment

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

This is a great example, but it doesn't output the time, which seems harder to implement, since a join("") wouldn't be enough. Would be great to have an example including time.

Copy link
Member Author

@rxaviers rxaviers Apr 11, 2017

Choose a reason for hiding this comment

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

since a join("") wouldn't be enough

Why? Check the parts output of a date and time case https://github.com/globalizejs/globalize/pull/700/files#diff-5e625c00d307f2b886e1b33225953053R165

Having said that, I think it's valid to include a date and time example.

Copy link
Member Author

@rxaviers rxaviers Apr 12, 2017

Choose a reason for hiding this comment

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

@rxaviers
Copy link
Member Author

rxaviers commented Apr 12, 2017

@jzaefferer thanks for your review

@rxaviers rxaviers force-pushed the date-to-parts-2 branch 2 times, most recently from d9b8c40 to 9408bff Compare Apr 12, 2017
@@ -4,6 +4,7 @@
"number-label": "Número",
"currency-label": "Moneda",
"date-label": "Fecha",
"date-to-parts-label": "Fecha (note el mes destacado en negro, el marcador de html se agregó utilizando dateToPartsFormatter)",
Copy link
Member Author

@rxaviers rxaviers Apr 12, 2017

Choose a reason for hiding this comment

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

Thanks @lwelti

@@ -4,6 +4,7 @@
"number-label": "Zahl",
"currency-label": "Währung",
"date-label": "Datum",
"date-to-parts-label": "Datum (beachten Sie den hervorgehobenen Monat, das Markup wurde mit dateToPartsFormatter hinzugefügt)",
Copy link
Member Author

@rxaviers rxaviers Apr 12, 2017

Choose a reason for hiding this comment

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

Thanks @jzaefferer

@rxaviers rxaviers merged commit 8eed86d into globalizejs:master Apr 12, 2017
2 checks passed
@rxaviers rxaviers deleted the date-to-parts-2 branch Apr 12, 2017
rxaviers added a commit to rxaviers/globalize that referenced this pull request May 17, 2017
When dateToPartsFormatter was added, dateFormatter became an alias to
it. Although globalize-compiler can handle it by using
`.compileExtracts()`, it doesn't by using `.compile()` and passing
`formattersAndParsers` argument. This update fixed that.

Amends e4234a7
Ref globalizejs#678
Ref globalizejs#697
Ref globalizejs#700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants