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

en-GB and en-US locales now use 12 hour format #214

Merged
merged 4 commits into from
Aug 13, 2017

Conversation

harogaston
Copy link
Contributor

@harogaston harogaston commented Aug 5, 2017

Original "HH:mm" (23:59) -> Now "hh:mm aa" (11:59 p.m.)

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md guide.
  • linted, built and tested the changes locally.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

Original "HH:mm" (23:59) -> Now "hh:mm aa" (11:59 p.m.)
@edcarroll
Copy link
Owner

Just want to hold off on this one until the 12:00 display is sorted for consistency, hope that's ok :)

@harogaston
Copy link
Contributor Author

Sorry, I did not get what you mean. Nevertheless I'm sure you are right. But I'm curious.

@edcarroll
Copy link
Owner

Sorry for the delay on getting an answer to what I mean!

So the issue with using aa and its siblings is the fact that they currently aren't correctly mapped to date-fns. If you look at the code that converts the locale values months, monthsShort e.t.c, it doesn't have any logic for converting time values.

Can you please add this in? Take a look at the date-fns locales for an example of how they encode the values

@harogaston
Copy link
Contributor Author

I'm sorry but I don't get your point. Can you give me an example of what doesn't work?

date-fns locales have this:

  var timeOfDayValues = {
    uppercase: ['AM', 'PM'],
    lowercase: ['am', 'pm'],
    long: ['a.m.', 'p.m.']
  }

  var localize = {
    ordinalNumber: ordinalNumber,
    weekday: buildLocalizeFn(weekdayValues, 'long'),
    weekdays: buildLocalizeArrayFn(weekdayValues, 'long'),
    month: buildLocalizeFn(monthValues, 'long'),
    months: buildLocalizeArrayFn(monthValues, 'long'),
    timeOfDay: buildLocalizeFn(timeOfDayValues, 'long', function (hours) {
      return (hours / 12) >= 1 ? 1 : 0
    }),
    timesOfDay: buildLocalizeArrayFn(timeOfDayValues, 'long')
  }

Is that timeOfDay what you are talking about?

@edcarroll
Copy link
Owner

Exactly - at the moment, ng2-semantic-ui takes the locale values, transforms them with custom versions of those functions (buildLocalizeFn , e.t.c.) and outputs a locale that date-fns understands.

At the moment, that doesn't include timeOfDayValues - so if someone wants to use it in a country that doesn't use PM, but instead GI (for the sake of this example), they can't currently just use aa.

Does that make sense?

@harogaston
Copy link
Contributor Author

Oh I see your point now. Sorry it took me a while. I'll try something during the weekend. Sorry about the repeated request for clarification.

@edcarroll
Copy link
Owner

No worries 😄 should be a quick one I hope, as I think you'll be able to reuse the functions in datepicker/helpers/date-fns.ts.

@edcarroll
Copy link
Owner

Ah also - when you've put a solution in place, please ping the various locale creators so they know to update their translations with the correct am-pm values (or steal them from date-fns 😉)

@edcarroll edcarroll modified the milestone: 0.9.5 Aug 11, 2017
@harogaston
Copy link
Contributor Author

@edcarroll Do you what to have all the alternatives that date-fns has [uppercase|lowercase|long] or just one that maps to all three?

var timeOfDayValues = {
  uppercase: ['AM', 'PM'],
  lowercase: ['am', 'pm'],
  long: ['a.m.', 'p.m.']
};

@edcarroll
Copy link
Owner

@harogaston let's go for all three, I feel clarity is better here

- timesOfDay
- timesOfDayUppercase
- timesOfDayLowercase
to support proper formatting/parsing of dates in datepicker.

Updated Localization page
Copy link
Owner

@edcarroll edcarroll left a comment

Choose a reason for hiding this comment

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

Awesome! Couple of final things before we get this merged (v small things tho)

@@ -4,8 +4,8 @@ const enUS:IPartialLocaleValues = {
datepicker: {
firstDayOfWeek: 0,
formats: {
time: "HH:mm",
datetime: "MMMM D, YYYY HH:mm",
time: "hh:mm aa",
Copy link
Owner

Choose a reason for hiding this comment

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

Does this invalidate the need for the other PR?

Copy link
Owner

Choose a reason for hiding this comment

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

Wait I'm an idiot ignore me

function buildLocalizeFn(values:IDateFnsLocaleValues, defaultType:string):DateFnsHelper<number, string> {
return (dirtyIndex, { type } = { type: defaultType }) => values[type][dirtyIndex];
function buildLocalizeFn(values:IDateFnsLocaleValues, defaultType:string,
indexCallback?:(oldIndex:number) => number):DateFnsHelper<number, string> {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you put each arg on its own line please? Leave a space before the 1st statement in the function also when breaking like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it

@edcarroll
Copy link
Owner

edcarroll commented Aug 13, 2017

Final thing regarding all of your commits, please make sure you are following the commit message style in the contribution guidelines - i.e. feat(datepicker): Added support for 12 hour locales

@harogaston
Copy link
Contributor Author

For better results it should be tested against feat(datepicker) Popup now honors locale and pickerLocaleOverrides but it works independently as well.

For now only two values for timeOfDay are supported, something like ["in the morning", "in the afternoon", "in the evening"] could be added later. it won't work just now because I think localization.service.ts:deepClone() and/or localization.service.ts:deepExtend do not work well with function objects which will be needed in locale definitions.

@edcarroll
Copy link
Owner

@harogaston have just merged that PR, do you want to pull master into this branch and do a quick test?

No worries re only 2 values - does date-fns support more than 2?

@harogaston
Copy link
Contributor Author

Final thing regarding all of your commits, please make sure you are following the commit message style in the contribution guidelines - i.e. feat(datepicker): Added support for 12 hour locales

Sorry about that. I also have a few other similar questions. I'll talk to you via gitter later.

@edcarroll
Copy link
Owner

Don't forgot to mark the review comments as done to close them 😄

@harogaston
Copy link
Contributor Author

harogaston commented Aug 13, 2017

@harogaston have just merged that PR, do you want to pull master into this branch and do a quick test?

I have tested it including those changes already.
I can still pull master if you want and do the merge.

No worries re only 2 values - does date-fns support more than 2?

Yes they allow things like:

var timeOfDayValues = {
  uppercase: ['AM', 'PM'],
  lowercase: ['am', 'pm'],
  long: ['du matin', 'de l’après-midi', 'du soir']
};

But for that to work locales will have to provide their own parsing functions.

@harogaston
Copy link
Contributor Author

Don't forgot to mark the review comments as done to close them 😄

I don't know how to...

Copy link
Owner

@edcarroll edcarroll left a comment

Choose a reason for hiding this comment

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

Hmm thought there was a button but may be mistaken. Oh well! Happy to approve this 🎉

@edcarroll
Copy link
Owner

Ahh ok let's leave the further customisation as a future feature 😄

@harogaston harogaston deleted the fix-locales-defaults branch August 13, 2017 02:59
mcosta74 pushed a commit that referenced this pull request Aug 14, 2017
* fix: Fixed AOT on SystemJS builder

Closes #209

* fix: Added custom FocusEvent interface (#231)

* fix: Added custom FocusEvent interface

Closes #202

FocusEvent isn't defined in UC browser, so added IFocusEvent with the property we need.

* style(util): Fixed tslint error

* chore(popup): Relaxed popper.js dependency (#228)

Follows up on #195

* fix(popup): Fixed delay causing destroyed view errors (#233)

Closes #189

* fix(select): Manually run change detector when option updates (#236)

* fix(select): Manually run change detector when option updates

Closes #213

* style(select): Fixed tslint error

* fix(select): Selected options now updated when options change (#232)

* fix(modal): Fixed aggressive autofocus sometimes causing errors (#237)

* fix(popup): Fixed conflict with BrowserAnimationsModule (#234)

* fix(popup): Fixed conflict with BrowserAnimationsModule

Closes #204

* style(popup): Fixed tslint error

* feat(popup): Added template context support (#238)

* fix(popup): Fixed focus events on popup (#243)

* feat(datepicker) Popup now honors locale and pickerLocaleOverrides (#215)

* Datepicker popup now honors locale and pickerLocaleOverrides

- Datepicker popup items now respect locale format
- Fixed a bug in zoom calendar mapping for datetime datepicker
- Fixed 'es' locale for consistency

(*) Partially addresses #164

* Added comments.

* Time of day values supported in locale definitions (#214)

* en-GB and en-US locales now use 12 hour format

Original "HH:mm" (23:59) -> Now "hh:mm aa" (11:59 p.m.)

* Fix locales Russian, Italian and Hebrew now extend from ILocaleValues.

* Added new IDatepickerFormatsLocaleValues fields:
- timesOfDay
- timesOfDayUppercase
- timesOfDayLowercase
to support proper formatting/parsing of dates in datepicker.

Updated Localization page

* Fixing code formatting

* feat(datepicker) Added initial date support for the datepicker (#216)

* feat(datepicker) New input pickerInitialDate

- New input (optional) that sets the intial date to display (null = today)
- Updated demo page

Partially addresses #165

* Now pickerInitialDate only sets CalendarService.currentDate property

* feat(datepicker): Initial date support
- Code formatting

* fix: Various minor bugfixes (#245)

* fix(select): Fixed destroyed view errors

* fix(modal): Fix modal auto closing when clicked

* fix(popup): Removed console log in focus handler

* fix(popup): Forced import of TemplateRef

* fix: Fixed AOT on SystemJS builder

Closes #209
gotenxds pushed a commit to gotenxds/ng2-semantic-ui that referenced this pull request Aug 15, 2017
* en-GB and en-US locales now use 12 hour format

Original "HH:mm" (23:59) -> Now "hh:mm aa" (11:59 p.m.)

* Fix locales Russian, Italian and Hebrew now extend from ILocaleValues.

* Added new IDatepickerFormatsLocaleValues fields:
- timesOfDay
- timesOfDayUppercase
- timesOfDayLowercase
to support proper formatting/parsing of dates in datepicker.

Updated Localization page

* Fixing code formatting
gotenxds pushed a commit to gotenxds/ng2-semantic-ui that referenced this pull request Aug 15, 2017
* fix: Fixed AOT on SystemJS builder

Closes edcarroll#209

* fix: Added custom FocusEvent interface (edcarroll#231)

* fix: Added custom FocusEvent interface

Closes edcarroll#202

FocusEvent isn't defined in UC browser, so added IFocusEvent with the property we need.

* style(util): Fixed tslint error

* chore(popup): Relaxed popper.js dependency (edcarroll#228)

Follows up on edcarroll#195

* fix(popup): Fixed delay causing destroyed view errors (edcarroll#233)

Closes edcarroll#189

* fix(select): Manually run change detector when option updates (edcarroll#236)

* fix(select): Manually run change detector when option updates

Closes edcarroll#213

* style(select): Fixed tslint error

* fix(select): Selected options now updated when options change (edcarroll#232)

* fix(modal): Fixed aggressive autofocus sometimes causing errors (edcarroll#237)

* fix(popup): Fixed conflict with BrowserAnimationsModule (edcarroll#234)

* fix(popup): Fixed conflict with BrowserAnimationsModule

Closes edcarroll#204

* style(popup): Fixed tslint error

* feat(popup): Added template context support (edcarroll#238)

* fix(popup): Fixed focus events on popup (edcarroll#243)

* feat(datepicker) Popup now honors locale and pickerLocaleOverrides (edcarroll#215)

* Datepicker popup now honors locale and pickerLocaleOverrides

- Datepicker popup items now respect locale format
- Fixed a bug in zoom calendar mapping for datetime datepicker
- Fixed 'es' locale for consistency

(*) Partially addresses edcarroll#164

* Added comments.

* Time of day values supported in locale definitions (edcarroll#214)

* en-GB and en-US locales now use 12 hour format

Original "HH:mm" (23:59) -> Now "hh:mm aa" (11:59 p.m.)

* Fix locales Russian, Italian and Hebrew now extend from ILocaleValues.

* Added new IDatepickerFormatsLocaleValues fields:
- timesOfDay
- timesOfDayUppercase
- timesOfDayLowercase
to support proper formatting/parsing of dates in datepicker.

Updated Localization page

* Fixing code formatting

* feat(datepicker) Added initial date support for the datepicker (edcarroll#216)

* feat(datepicker) New input pickerInitialDate

- New input (optional) that sets the intial date to display (null = today)
- Updated demo page

Partially addresses edcarroll#165

* Now pickerInitialDate only sets CalendarService.currentDate property

* feat(datepicker): Initial date support
- Code formatting

* fix: Various minor bugfixes (edcarroll#245)

* fix(select): Fixed destroyed view errors

* fix(modal): Fix modal auto closing when clicked

* fix(popup): Removed console log in focus handler

* fix(popup): Forced import of TemplateRef

* fix: Fixed AOT on SystemJS builder

Closes edcarroll#209
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.

2 participants