-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Conversation
BREAKING CHANGE: Applications must being using Angular 1.6
@wesleycho, what's the timeline for this being merged and a new release being cut? We are still using Bower for dependencies, so I'm in search of the path of least resistance for myself to get this code into our app. |
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.
There are a couple of areas of concern here, mainly with this being a strict compatibility break with 1.5.
"angular-sanitize": "1.5.8", | ||
"angular": "1.6.0", | ||
"angular-mocks": "1.6.0", | ||
"angular-sanitize": "1.6.0", |
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.
These can all probably be bumped to 1.6.1
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.
done
|
||
// need to start with existing ModelOptions but also give it greatest precedence minus its default of null timezone | ||
ngModelOptions = ngModelCtrl_.$options | ||
.createChild(datepickerConfig.ngModelOptions) |
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.
This breaks compatibility with 1.5 I believe - we cannot have that for a minor version bump. Is there no way to made this compatible with both versions?
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.
done
if ($scope.datepickerOptions.initDate) { | ||
self.activeDate = dateParser.fromTimezone($scope.datepickerOptions.initDate, ngModelOptions.timezone) || new Date(); | ||
self.activeDate = dateParser.fromTimezone($scope.datepickerOptions.initDate, ngModelOptions.getOption('timezone')) || new 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.
Ditto with getOption
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.
done
This LGTM minus the compatibility break - if this is to be merged earlier, then we absolutely cannot break compatibility yet. A mechanism for handling the ngModel.$options differences needs to be created to handle differences in 1.5 and 1.6. |
@wesleycho I'm pretty confident things are good to go. As you can see, the tests passed. Locally, I also reset my version of Angular back to 1.5 and re-ran the tests. All tests in regards to $options still passed. The only failure, which was expected, was in regards to 1.5 adding the extra span for transclusion--I was surprised the other updated transclusion tests didn't fail. |
Thanks - I will take another pass over the weekend (sleep deprived currently) and if things are good, I will merge & cut a release then. |
Updating code and tests to address #6360, making angular-ui/bootstrap compatible with Angular 1.6. These are BREAKING CHANGES, incompatible with Angular 1.5.