-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
fix(datepicker): fix wrong datepicker-input value for non MM/DD/YYYY locales #6798
Conversation
@@ -226,6 +227,7 @@ export class MdDatepickerInput<D> implements AfterContentInit, ControlValueAcces | |||
this._datepickerSubscription = | |||
this._datepicker.selectedChanged.subscribe((selected: D) => { | |||
this.value = selected; | |||
// console.log('here ' + this.value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove console log ?
@@ -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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I modified The only thing that still can't be fixed in this change is when the user types in the date in DD/MM/YYYY locales because |
@@ -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; |
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
…locales (angular#6798) * fix date change value for internationalization * change value setting logic + add test
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
After internationalization, the value is either null or wrong. For example, 06.09.2017 which is supposed to be September 6th, 2017 in European languages would be parsed as June 9th, 2017. The setter of
value
calls_getValidDateOrNull()
to get the valid date from the given Date object, so just returning the value saved from the setter in getter would return the correct datepicker input value.