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

Manage request as an upload #243

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Westbrook
Copy link

This is also in reference to #235.

This is a first pass at this PR as I have a number of questions about what form a more complete implementation should be.

Here I've added an isUpload property to both the iron-ajax and iron-upload so that when it is set to true the listener in iron-request that updates the progress property is attached to xhr.upload instead of just xhr. This allows the implementer to track file uploads into the UI, etc.

First, I'm wondering if when uploading all events tracked in send() should be pointed to xhr.upload. Second, should this actually be an even greater change where the progress object be duplicated into uploadProgress and downloadProgress so that both objects and listeners can be tracked at all times?

Once that is figured out, I'd look for insight on good ways to add tests for this. There doesn't seem to be any test in iron-request for progress, and this seems like it might be a good place to do so?

@Westbrook
Copy link
Author

Any update on this?

@Westbrook
Copy link
Author

Ping?

@btelles
Copy link

btelles commented Mar 9, 2017

@Westbrook FWIW, I implemented this on my own and for some reason the progress property is not propagating up to the iron-ajax element. very weird.

@Westbrook
Copy link
Author

Westbrook commented Mar 9, 2017

@btelles Implemented from my fork? Did you instal via bower.json? I had a pita of a time getting bower to recognize that as the version to use vs the ones pointed to in other dependencies. If you are working from my code I'd be glad to hear your experience and see if we can't find the right changes to support a wide set of users for this. I actually just released my feature based on these changes this morning, but I could have easily been working around something on accident in my own code.

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

Successfully merging this pull request may close these issues.

3 participants