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.

Sign up for GitHub

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

fix(datepicker): fix wrong datepicker-input value for non MM/DD/YYYY locales #6798

Merged
merged 2 commits into from
Sep 11, 2017
Merged

fix(datepicker): fix wrong datepicker-input value for non MM/DD/YYYY locales #6798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4016c96
fix date change value for internationalization
jwshinjwshin Sep 1, 2017
52525a6
change value setting logic + add test
jwshinjwshin Sep 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/lib/datepicker/datepicker-input.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces
/** The value of the input. */
@Input()
get value(): D | null {
return this._getValidDateOrNull(this._dateAdapter.parse(
this._elementRef.nativeElement.value, this._dateFormats.parse.dateInput));
return this._value;
Copy link
Member

Choose a reason for hiding this comment

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

Add a unit test for this fix?

Copy link
Contributor

@mmalerba mmalerba Sep 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, if we're going to make this change we need to have tests that ensure the cached value returned by the getter never gets out of sync with the value of the element. We would need to test that value is correct after:

  • update due to [value] binding
  • update due to ngModel change
  • update due to formControl change
  • update due to valid user input
  • update due to invalid user input
  • possibly update due to document.query('input').value =? (this change will probably break this case, but maybe that ok because people shouldn't be doing that)

Also note that this will not fix the problem of the datepicker not working in non MM/DD/YYYY locales, it will still break as soon as the user inputs something because it has to go through DateAdapter.parse. The real fix for this is to integrate the new Angular i18n API that will be part of Angular 5.0 or have people use a different adapter in the mean time such as the MomentDateAdapter that I'm currently working on. However if this change doesn't break any of the cases I listed, I would be ok with merging it as it would save us some parsing work every time the value is read.

}
set value(value: D | null) {
if (value != null && !this._dateAdapter.isDateInstance(value)) {
Expand All @@ -123,12 +122,14 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces
value = this._getValidDateOrNull(value);

let oldDate = this.value;
this._value = value;
this._renderer.setProperty(this._elementRef.nativeElement, 'value',
value ? this._dateAdapter.format(value, this._dateFormats.display.dateInput) : '');
if (!this._dateAdapter.sameDate(oldDate, value)) {
this._valueChange.emit(value);
}
}
private _value: D | null;

/** The minimum valid date. */
@Input()
Expand Down Expand Up @@ -285,6 +286,7 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces
let date = this._dateAdapter.parse(value, this._dateFormats.parse.dateInput);
this._lastValueValid = !date || this._dateAdapter.isValid(date);
date = this._getValidDateOrNull(date);
this._value = date;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just go through the setter? A lot of this logic looks the same. I think you could change this method to:

let date = this._dateAdapter.parse(value, this._dateFormats.parse.dateInput);
this.value = date;
this._cvaOnChange(this.date);
this.dateInput.emit(new MdDatepickerInputEvent(this, this._elementRef.nativeElement));

Copy link
Author

Choose a reason for hiding this comment

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

@mmalerba this.value = date can't be used in _onInput() because the setter of value formats the date and sets it as the property on the native element so simply typing in 1 on immediately trigger update and set the value to 1.1.2001.

this._renderer.setProperty(this._elementRef.nativeElement, 'value', value ? this._dateAdapter.format(value, this._dateFormats.display.dateInput) : '');

We would only want the DateAdapter.format update when the date is chosen by clicking on a date in datepicker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see right, that makes sense

this._cvaOnChange(date);
this._valueChange.emit(date);
this.dateInput.emit(new MdDatepickerInputEvent(this, this._elementRef.nativeElement));
Expand Down
55 changes: 54 additions & 1 deletion src/lib/datepicker/datepicker.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ import {MdDatepicker} from './datepicker';
import {MdDatepickerInput} from './datepicker-input';
import {MdInputModule} from '../input/index';
import {MdNativeDateModule} from '../core/datetime/index';
import {DEC, JAN} from '../core/testing/month-constants';
import {DEC, JAN, SEP} from '../core/testing/month-constants';
import {createKeyboardEvent, dispatchEvent} from '@angular/cdk/testing';
import {MdFormFieldModule} from '../form-field/index';
import {MAT_DATE_LOCALE} from '../core/datetime/date-adapter';
import {NativeDateModule} from '../core/datetime/index';

describe('MdDatepicker', () => {
afterEach(inject([OverlayContainer], (container: OverlayContainer) => {
Expand Down Expand Up @@ -943,6 +945,45 @@ describe('MdDatepicker', () => {
});

});

describe('internationalization', () => {
let fixture: ComponentFixture<DatepickerWithi18n>;
let testComponent: DatepickerWithi18n;
let input: HTMLInputElement;

beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [
MdDatepickerModule,
MdFormFieldModule,
MdInputModule,
MdNativeDateModule,
NoopAnimationsModule,
NativeDateModule,
FormsModule
],
providers: [{provide: MAT_DATE_LOCALE, useValue: 'de-DE'}],
declarations: [DatepickerWithi18n],
}).compileComponents();

fixture = TestBed.createComponent(DatepickerWithi18n);
fixture.detectChanges();
testComponent = fixture.componentInstance;
input = fixture.nativeElement.querySelector('input') as HTMLInputElement;
}));

it('should have the correct input value even when inverted date format', () => {
let selected = new Date(2017, SEP, 1);
testComponent.date = selected;
fixture.detectChanges();

fixture.whenStable().then(() => {
fixture.detectChanges();
expect(input.value).toBe('01.09.2017');
expect(testComponent.datepickerInput.value).toBe(selected);
});
});
});
});


Expand Down Expand Up @@ -1099,3 +1140,15 @@ class DatepickerWithChangeAndInputEvents {

onDateInput() {}
}

@Component({
template: `
<input [mdDatepicker]="d" [(ngModel)]="date">
<md-datepicker #d></md-datepicker>
`
})
class DatepickerWithi18n {
date: Date | null = new Date(2010, JAN, 1);
@ViewChild('d') datepicker: MdDatepicker<Date>;
@ViewChild(MdDatepickerInput) datepickerInput: MdDatepickerInput<Date>;
}