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): close calendar after choose the same date again #6323

Merged
merged 6 commits into from
Aug 15, 2017

Conversation

hwesol13
Copy link
Contributor

@hwesol13 hwesol13 commented Aug 7, 2017

Selecting already selected date didn't close datepicker. This contains functionality to close datepicker even when already selected date is picked.

@hwesol13 hwesol13 requested a review from mmalerba as a code owner August 7, 2017 12:36
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 7, 2017
@mmalerba
Copy link
Contributor

mmalerba commented Aug 7, 2017

Though this may be the easiest way to get the functionality you want, I don't think it's really correct to emit a selectedChange event when the selected date has not actually changed. Instead I would separate the responsibilities into two separate observables selectedChange would just responsible for updating the value, not closing the calendar. And the new observable (maybe call it userSelection or something) would be responsible for closing the calendar regardless of whether its the same value.

@hwesol13
Copy link
Contributor Author

hwesol13 commented Aug 8, 2017

Nice one. Thx for advice.

@@ -6,5 +6,6 @@
[maxDate]="datepicker._maxDate"
[dateFilter]="datepicker._dateFilter"
[selected]="datepicker._selected"
(selectedChange)="datepicker._selectAndClose($event)">
(selectedChange)="datepicker._selectAndClose($event)"
Copy link
Contributor

Choose a reason for hiding this comment

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

This _selectAndClose method and its comment should be updated since it no longer closes

(selectedChange)="_dateSelected($event)">
(selectedChange)="_dateSelected($event)"
(userSelection)="_userSelected()"
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Closing bracket should be on line above

this._onTouched();
this.dateInput.emit(new MdDatepickerInputEvent(this, this._elementRef.nativeElement));
this.dateChange.emit(new MdDatepickerInputEvent(this, this._elementRef.nativeElement));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think they actually prefer line breaks like this to stay 4 spaces

@@ -67,6 +67,9 @@ export class MdMonthView<D> implements AfterContentInit {
/** Emits when a new date is selected. */
@Output() selectedChange = new EventEmitter<D | null>();

/** Emits when date is picked */
@Output() userSelection = new EventEmitter();
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment you had above seems more appropriate for this as well:

/** Emits when any date is selected. */

Nit: Add a period to both to match other comments

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I think it should have a <void> type?

new EventEmitter<void>();

Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

+1 to @willshowell's suggestions and a couple comments on the test

@@ -190,6 +190,26 @@ describe('MdDatepicker', () => {
});
});

it('setting twice to the same selected value should update input and close calendar', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

'clicking the currently selected date should close the calendar without firing selectedChanged'

expect(testComponent.datepickerInput.value).toEqual(new Date(2020, JAN, currentDay));

let cells = document.querySelectorAll('.mat-calendar-body-cell');
dispatchMouseEvent(cells[1], 'click');
Copy link
Contributor

Choose a reason for hiding this comment

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

verify that selectedChanged only emits the first time

@@ -224,13 +224,12 @@ export class MdDatepicker<D> implements OnDestroy {
}

/** Selects the given date and closes the currently open popup or dialog. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you update this comment too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh... sorry, i missed it

@@ -224,7 +224,7 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces
ngAfterContentInit() {
if (this._datepicker) {
this._datepickerSubscription =
this._datepicker.selectedChanged.subscribe((selected: D) => {
this._datepicker.selectedChanged.subscribe((selected: D) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this should stay the same too. They indent 2 for function blocks and 4 for line breaks

Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM, will add merge-ready label once nits are addressed

@@ -190,6 +190,30 @@ describe('MdDatepicker', () => {
});
});

it('clicking the currently selected date should close the calendar' +
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space before close of string so result doesn't read: "calendarwithout"

@mmalerba
Copy link
Contributor

great, and we can just ignore that screenshot test failure, it's just because the blinking input caret

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Aug 11, 2017
@andrewseguin andrewseguin merged commit 9ba5d84 into angular:master Aug 15, 2017
@hwesol13 hwesol13 deleted the closing-date-picker branch August 17, 2017 14:29
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants