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

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

Merged
merged 2 commits into from
Aug 13, 2017

Conversation

harogaston
Copy link
Contributor

  • 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

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.

- 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
@edcarroll
Copy link
Owner

Thank you for the PR 🎉 will get this looked at properly tomorrow 😄

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.

Looks good, couple of very minor points

@@ -24,7 +24,7 @@ const es:IPartialLocaleValues = {
],
formats: {
time: "HH:mm",
datetime: "D MMMM [de] YYYY HH:mm",
datetime: "D [de] MMMM [de] YYYY HH:mm",
Copy link
Owner

Choose a reason for hiding this comment

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

@genuinefafa is this correct?

Copy link
Owner

Choose a reason for hiding this comment

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

@harogaston I know you live in Uruguay just want to check all the same!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am sure. Further reference here (in Spanish sorry) The website belongs to the Spanish Royal Academy. Which is the authority that legislates the language (yes, we are cool as that :P).

Copy link
Owner

Choose a reason for hiding this comment

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

Great! All good then 😄

Choose a reason for hiding this comment

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

as @harogaston said, we - the ones from the Río de la Plata - know better (?).

@@ -69,7 +69,7 @@ export class DatetimeMappings extends CalendarMappings {
[CalendarViewType.Month, CalendarViewType.Year],
[CalendarViewType.Date, CalendarViewType.Month],
[CalendarViewType.Hour, CalendarViewType.Date],
[CalendarViewType.Minute, CalendarViewType.Date]
[CalendarViewType.Minute, CalendarViewType.Hour]
Copy link
Owner

Choose a reason for hiding this comment

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

This was on purpose as it is the behaviour of the original module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then. For me it felt like a bug, meaning odd behavior.

Copy link
Owner

Choose a reason for hiding this comment

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

Fairs, @mcosta74 what are your thoughts on this behaviour?

Copy link
Contributor Author

@harogaston harogaston Aug 12, 2017

Choose a reason for hiding this comment

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

I'll wait for this to be decided on, before I send the PR.

Points in favor of the proposed change:

  1. "zoomOut" shouldn't skip a step
  2. If user wants to change hour, will be forced to re-select date

Points against the proposed change:

  1. After a date is selected, two clicks are needed to get to the date view (if on datetime mode)

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, let's keep this change. Because, in the Calendar module (that this is based on) the left right arrows go across days when looking at minutes, which fits the old behavior. However, the datepicker allows navigation across hours, so it's inconsistent now.

import { CalendarView, CalendarViewType } from "./calendar-view";
import { CalendarItem } from "../directives/calendar-item";
import { CalendarRangeService } from "../services/calendar-range.service";
import { DateParser } from "../classes/date-parser";

export class CalendarRangeHourService extends CalendarRangeService {
public configureItem(item:CalendarItem, baseDate:Date):void {
item.humanReadable = `${Util.String.padLeft(item.date.getHours().toString(), 2, "0")}:00`;
const customFormat:string = this.service.localeValues.formats.time.replace(/m/g, "0");
Copy link
Owner

Choose a reason for hiding this comment

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

Please add a comment saying what this does for future clarity (took me a minute to get it!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right!

const dateTimeFormat:string = this.service.localeValues.formats.datetime.replace(/m/g, "0");
return new DateParser(dateTimeFormat, this.service.localeValues).format(this.currentDate);
} else {
const timeFormat:string = this.service.localeValues.formats.time.replace(/m/g, "0");
Copy link
Owner

Choose a reason for hiding this comment

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

As above, add a quick explanation here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

@edcarroll edcarroll modified the milestone: 0.9.5 Aug 11, 2017
@harogaston harogaston changed the title Datepicker popup now honors locale and pickerLocaleOverrides feat(datepicker) Popup now honors locale and pickerLocaleOverrides Aug 13, 2017
@edcarroll edcarroll merged commit c0158b3 into edcarroll:master Aug 13, 2017
@harogaston harogaston deleted the 12-hour-time-selection 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
…dcarroll#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.
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.

3 participants