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

Add other dp events, and fix possible infinite digest loop #28

Closed
wants to merge 10 commits into from
Closed

Add other dp events, and fix possible infinite digest loop #28

wants to merge 10 commits into from

Conversation

CapitanMorgan
Copy link

Adds the previously unimplemented items from http://eonasdan.github.io/bootstrap-datetimepicker/Events/

Fixes digest loop i mentioned in #26 and by doing so allows for the functionality desired in #26

@atais
Copy link
Owner

atais commented Jun 23, 2017

@CapitanMorgan whoa, why the tests failed so badly ?

@CapitanMorgan
Copy link
Author

CapitanMorgan commented Jul 7, 2017

When i check out master locally, it was already broken. So I started about trying to fix it.

Edit: it appeared to be mainly due to issues with how i was running it , not sure if its because of windows or git bash. Eventually once i got it running i was able to fix them to work with the new changes locally. I still need to figure out how if possible to add something to test that the infinite digest loop doesn't still occur. At minimum i want to make a js fiddle showing what was happening when you use certain settings with the prior version.

@atais
Copy link
Owner

atais commented Jul 7, 2017

You are right. So at the time of last commit to master Travis was OK, but actually, now it does not pass.
https://travis-ci.org/atais/angular-eonasdan-datetimepicker/builds/197936266

So I think 1st thing would be to sort this out and later I can think of adding new stuff.

FFS, JavaScript world could not be stable for even couple of months 😠

@atais
Copy link
Owner

atais commented Jul 7, 2017

I fixed the tests in #29

@@ -25,31 +25,41 @@
<div class="col-md-3">
<div class="form-group">
<div class="input-group" datetimepicker
on-change="vm.optionsTo.minDate = vm.dateFrom"
on-change="vm.onChangeDatePickerFrom()"
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep the old tests untouched.

  1. All new features are optional, so I believe the old scenarios should work?
  2. Please provide NEW tests for new features

@@ -23,9 +23,7 @@
"scripts": {
"pretest": "npm install",
"test": "(npm start > /dev/null &) && (gulp protractor)",

Copy link
Owner

Choose a reason for hiding this comment

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

Those changes are probably not necessary as well.

} else if ((!d1 && d2 && (d1 === d1)) || (d1 && !d2 && (d2 === d2))){
return false; // the d1 === d1 is to avoid NaN which is passed in iniitilization
}
return true; // return true for invald scenarios to prevent possible infinite digest loop
Copy link
Owner

Choose a reason for hiding this comment

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

what are the invalid scenarios?
and why would you return true on isDateEqual check when they are invalid?

If properties are not date objects, or whatever is they reason why they can't be checked - I think- this method should return false.

@atais
Copy link
Owner

atais commented Jul 7, 2017

So ideally I would like you to create a test scenario describing this infinite loop that would fail on current master.
And from there you could create a fix for it and present the whole as a PR.

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

Successfully merging this pull request may close these issues.

2 participants