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) New input pickerInitialDate #216

Merged
merged 3 commits into from
Aug 13, 2017

Conversation

harogaston
Copy link
Contributor

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

Partially addresses #165

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.

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

Partially addresses edcarroll#165
@edcarroll
Copy link
Owner

Thanks @harogaston, will get this looked at and merged hopefully 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.

Thank you for looking at this issue 😄 comments outline a simpler way of doing this, already built into the datepicker - will merge once changed to use that :)

@@ -148,6 +151,18 @@ export class SuiDatepickerDirective
}
}

public ngOnInit():void {
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than going this way, make use of the pre existing currentDate property of the CalendarService.

In the datepicker directive, add the following just before you set the service selected date:

this.componentInstance.service.currentDate = this.initialDate || new Date()

Then in the calendar service, change reset to something like:

public reset():void {
    this.currentView = this.config.mappings.finalView;

    if (!this._selectedDate) {
        let current = this.currentDate.getTime();
        if (this._minDate) {
            current = Math.max(current, this._minDate.getTime());
        }
        if (this._maxDate) {
            current = Math.min(current, this._maxDate.getTime());
        }

        this.currentDate = new Date(current);
        this.config.updateBounds(this.currentDate);

        this.currentView = this.config.mappings.initialView;
    }
}

That should do the trick without needing to do any null checks etc 😄

Copy link
Contributor Author

@harogaston harogaston Aug 11, 2017

Choose a reason for hiding this comment

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

The thing is, as I imagined it, that if [pickerInitialDate] was set, that meant that the input should be pre populated (i.e. this._selectedDate set) with the given date, or today() if null was passed, or do nothing (i.e. same as previous behavior) if [pickerInitialDate] was not used or was undefined.

Your proposal only concerns with the date the user will see when opening the pop up. Which is also interesting. Which way to go?

I personally think that if we allow for an pickerInitialDate that date should be selected. But it is just preference/semantics.

PS: Nevertheless, the code could still be improved.

@@ -95,6 +95,11 @@ export class DatepickerPage {
defaultValue: "datetime"
},
{
name: "pickerInitialDate",
type: "Date",
description: "Sets the intial date to display (null = today)."
Copy link
Owner

Choose a reason for hiding this comment

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

Change to Sets the intial date to display when no date is selected.. Add new Date() as the defaultValue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha (but also read bottom comment)

@harogaston
Copy link
Contributor Author

harogaston commented Aug 11, 2017

Some thoughts.

Regarding new Date() as the defaultValue and considering what I mentioned before: Maybe the intended behavior it was not clear. As I implemented it, there is a difference between

  • [pickerInitialDate]="null" means: initial date is today
  • [pickerInitialDate] not present or undefined means: do nothing (same old behavior)
  • [pickerInitialDate]="dateObject" means: set dateObject as the selected date (the input will be populated!)

That was my idea. Maybe we should get to an agreement regarding the expected behavior before we do anything else.

@edcarroll
Copy link
Owner

On those bullets:

1 & 2: the old behavior is new Date() but bounded to possible values, i.e. inside the min & max date range. This means that you don't get the datepicker into a ridiculous range if your min & max are waaaay in the past / future, but also you don't need to configure it.
3: In that case, why not just use pickerSelectedDate? Having this initial date property set a selected date is very confusing - do we then emit that date in ngModel? I don't like the idea of two separate properties essentially conflicting with eachother.

Do you see what I mean?

@harogaston
Copy link
Contributor Author

I'm sorry, you are absolutely right. I was confused. I'll get onto it this weekend!

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

harogaston commented Aug 12, 2017

Just did what you suggested. Sorry about the confusion.

Should I have merged new changes from master before this commit?

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.

Minor points, literally 30 second fixes. Will merge as soon as that's done 😄

@@ -12,6 +11,7 @@ import { IDatepickerLocaleValues, RecursivePartial, SuiLocalizationService } fro
import { SuiPopupComponentController, PopupAfterOpen, PopupConfig, PopupTrigger } from "../../popup";
import { SuiDatepicker, DatepickerMode } from "../components/datepicker";
import { CalendarConfig, YearConfig, MonthConfig, DatetimeConfig, TimeConfig, DateConfig } from "../classes/calendar-config";
import { isValid } from "date-fns";
Copy link
Owner

Choose a reason for hiding this comment

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

Unnecessary?

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, just went unnoticed by the analyzer, idk.

Copy link
Owner

Choose a reason for hiding this comment

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

So the unused imports option is currently disabled because it's super unreliable... So just trying to delete things when I see them really until the analyzer is fixed.

@@ -101,15 +101,15 @@ export class CalendarService {
this.currentView = this.config.mappings.finalView;

if (!this._selectedDate) {
let today = new Date().getTime();
let current = this.currentDate ? this.currentDate.getTime() : new Date().getTime();
Copy link
Owner

Choose a reason for hiding this comment

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

Set currentDate to new Date() in the constructor, and make this line just this.currentDate.getTime() since currentDate isn't nullable.

Copy link
Owner

Choose a reason for hiding this comment

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

Just to make it look a bit nicer

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! 💃

@edcarroll edcarroll merged commit 35f98b6 into edcarroll:master Aug 13, 2017
@harogaston harogaston deleted the datepicker-default-date branch August 13, 2017 03:00
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
…roll#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
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.

None yet

2 participants