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): change trigger event for NgbInputDatepicker #1605

Closed
wants to merge 4 commits into from
Closed

fix(datepicker): change trigger event for NgbInputDatepicker #1605

wants to merge 4 commits into from

Conversation

PerfectPixel
Copy link
Contributor

Changes the trigger event in NgbInputDatepicker for manual date change from change to inputas discussed in #1225.

@pkozlowski-opensource
Copy link
Member

@PerfectPixel thnx for the PR, it looks good. One question though - did you test it on IE10/11 and / or Edge? I recall that there are some issues with how the input event is handled in those browsers and I would like to verify that we are not breaking code on those browsers (existing unit tests won't catch those issues, unfortunately...)

@PerfectPixel
Copy link
Contributor Author

PerfectPixel commented Jun 5, 2017

@pkozlowski-opensource ok there is a rub. This line causes the following behaviour:

  1. User types "2017"
  2. "2" is not a valid NgbDate -> this._model = null, therefore, _onChange = '2' and the input is overwritten with null by _writeModelValue(null)
  3. "0" becomes the first number again as the input was reset with null.
  4. and so on...

Here is a gif where I am typing numbers randomly.
untitled

The easiest solution is to uncomment this line. However, there are a few changes in behaviour:

  1. Typing a year + month and press enter no longer autocompletes the input to year + month + 1st of month (although the model stays the same).
  2. Lets you input letters.
  3. If you have a valid date like 2017-06-05 and delete the 05, you'll end up with 2017-06- as the model. If you delete the last dash, you get a valid NgbDate again.

Edit: The input event itself works fine using IE 11 and Edge

@PerfectPixel
Copy link
Contributor Author

@pkozlowski-opensource have a look at my last commit. This fixes the input. Also, it mitigates the first issue as I introduced a change event that updates the view with the model enabling autocomplete of the day once again. The 2nd and 3rd issues are also happening in the original version just on a different event.

@pkozlowski-opensource
Copy link
Member

@PerfectPixel it all looks good to me! We've got a big PR for date picker in-flight (keyboard navigation) so I'm going to wait with merging till the end of the week (so other PR can land first).

Thnx!

@pkozlowski-opensource
Copy link
Member

@PerfectPixel the other PR I've mentioned just landed so we can get in this one. Would you mind rebasing so we can land your PR?

@PerfectPixel
Copy link
Contributor Author

PerfectPixel commented Jul 11, 2017

Not at all, I'll have a look at it tonight

@PerfectPixel
Copy link
Contributor Author

@pkozlowski-opensource all done :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants