-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1986 +/- ##
========================================
Coverage 55.33% 55.33%
========================================
Files 48 48
Lines 3273 3273
Branches 384 384
========================================
Hits 1811 1811
Misses 1446 1446
Partials 16 16
Continue to review full report at Codecov.
|
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's a lot to cover with the updated Deferred
object API and behavior, so it makes sense to me to hold off on more changes until this upgrade is in place first, if needed. If that sounds good to you too @noahmanger then this is good to merge!
@@ -137,7 +137,7 @@ DownloadItem.prototype.refresh = function() { | |||
contentType: 'application/json', | |||
dataType: 'json' | |||
}); | |||
this.promise.then(this.handleSuccess.bind(this)); | |||
this.promise.done(this.handleSuccess.bind(this)); |
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.
Interesting, this required a change to done
? In reading the guide I would've thought then
would still be preferred as that is the new way forward with the Deferred
object changes. It appears we're still using the old style, though, according to this part, which would make sense given this change.
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.
Yep, I was confused about this too. But there was an issue with the wrong context not being passed and this solution fixed it. Since it's a test and not a feature, seemed ok to just leave it.
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.
Sounds good to me! Should we work in the fec-style
version bump in this PR to just ensure full testing of the updates?
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.
Sounds good.
This updates jQuery to the latest version. I clicked through the site and everything seems to be working. I also ready through the upgrade guide and checked for instances of the things they warned about and I didn't find anything concerning.
The one snag is that a single test is failing, which seems to perhaps relate to the change in the$.Deferred()
implementation.But I'm stumped, so I'd appreciate if someone else could take a look.Update: the test issue is resolved.