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

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

Merged
merged 2 commits into from
Sep 11, 2017

Conversation

g1shin
Copy link

@g1shin g1shin commented Sep 1, 2017

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.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 1, 2017
@@ -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);
Copy link
Contributor

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;
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

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.

@g1shin
Copy link
Author

g1shin commented Sep 6, 2017

I modified _onInput method so that input event does not result in null value for MdDatepickerInput. I also added a unit test for the internationalization fix. All the other instances that @mmalerba mentioned that need to be checked already seem to have tests for them, and they are all passing; so I think the value is correct now.

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 _onInput simply parses in the input string. But with DateAdapter.parse, I don't think there is a way to address this particular case right now.

@@ -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

@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 6, 2017
@g1shin g1shin changed the title fix(datepicker): fix null datepicker value after internationalization fix(datepicker): fix wrong datepicker-input value for non MM/DD/YYYY locales Sep 6, 2017
@mmalerba mmalerba merged commit 29399b8 into angular:master Sep 11, 2017
josephperrott pushed a commit to josephperrott/components that referenced this pull request Sep 15, 2017
…locales (angular#6798)

* fix date change value for internationalization

* change value setting logic + add test
@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 7, 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