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

[bz 6440446] Bug with cancel upload #1053

Merged
merged 2 commits into from Sep 17, 2013

Conversation

Projects
None yet
3 participants
@JetFault
Copy link
Contributor

JetFault commented Aug 1, 2013

When canceling an upload right as it finishes, cancelUpload throws a TypeError because this.get('xhr') is null.

@JetFault

This comment has been minimized.

Copy link
Contributor

JetFault commented Aug 2, 2013

also fixed typo where uploadcancel event wasn't being fired in commit 9db6351 :(

@ericf

View changes

build/file-html5/file-html5.js Outdated
this.get('xhr').abort();
try {
this.get('xhr').abort();
} catch(e) {

This comment has been minimized.

@ericf

ericf Aug 5, 2013

Member

Instead of a try/catch, could you just check whether or not the result of calling get('xhr') is truthy before calling abort()?

var xhr = this.get('xhr');
if (xhr) {
    xhr.abort();
}
@@ -233,7 +233,7 @@ Y.extend(UploaderQueue, Y.Base, {
updatedEvent.originEvent = event;
updatedEvent.file = event.target;

this.fire("uploadcacel", updatedEvent);
this.fire("uploadcancel", updatedEvent);

This comment has been minimized.

@ericf

ericf Aug 5, 2013

Member

Yikes! Good catch!

@ericf

This comment has been minimized.

Copy link
Member

ericf commented Aug 5, 2013

@JetFault Thanks for working on this. I added a comment about the try/catch above. Could you also add a test and HISTORY.md entry for this?

JetFault added some commits Aug 2, 2013

fix typo
firing wrong event.
expected `uploadcancel`
Fixed canceling upload on finished xhr
Updated History.md
Wrote unit tests for file-html5 uploader queue
@JetFault

This comment has been minimized.

Copy link
Contributor

JetFault commented Aug 8, 2013

@ericf added tests, changed to if/else

@JetFault

This comment has been minimized.

Copy link
Contributor

JetFault commented Sep 16, 2013

@ericf Any reason why this isn't getting merged in?

@clarle

This comment has been minimized.

Copy link
Contributor

clarle commented Sep 17, 2013

@ericf This looks good to me, I'll handle the merge conflicts for getting this in if you sign this off.

@clarle clarle merged commit 70a51be into yui:dev-master Sep 17, 2013

1 check passed

default The Travis CI build passed
Details
@clarle

This comment has been minimized.

Copy link
Contributor

clarle commented Sep 17, 2013

@JetFault I re-tested this on IE, Chrome, and Firefox and all looks good. Fixed up some of your HISTORY.md entries but otherwise merged in as-is.

Thanks again for the contribution, we really appreciate it!

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