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

Do not reset input when upload fails (single-upload mode) #985

Closed
wants to merge 2 commits into from

Conversation

x-yuri
Copy link

@x-yuri x-yuri commented May 27, 2017

Scope

This pull request includes a

  • Bug fix
  • New feature
  • Translation

Changes

The following changes were made

  • input is not reset when upload fails (single-upload mode)

Related Issues

Multiple Ajax Upload does not resend the file after an error
Input file is reset when upload fails

Discussion

It's not finished yet. What's left is to fix the preview part. But... do we need that for use case at hand (not just uploading files, but form with a file input)? Your plugin allows to have some fixed (initial) thumbs, that doesn't get overwritten. Thumb list may act as a log (keeps everything you selected or uploaded). And whatnot. But for forms with file inputs even preview might not be necessary. Or if necessary, that would generally be just one image in overwrite mode, so to say. What do you say?

@kartik-v
Copy link
Owner

@x-yuri - if you are using it as a native FILE input then the changes in the PR are not correct.

For using it like a native file input - the developer should not be setting the uploadUrl - in that case the ajax upload methods is not applicable and methods like updateStack, addToStack etc. are not applicable.

When using it as a native file input - you do not set the uploadUrl... just handle the error in your form submission code... and it should work like any native file input. No changes are required - unless you explicitly reset the form.

@kartik-v
Copy link
Owner

kartik-v commented May 28, 2017

With ajax upload mode... there is one possibility to look for a bit of enhancement - but it will be only for this scenario

  • multiple attribute should be false - means user can always select one file
  • overwriteInitial should be set to true - to overwrite preview thumbs and not append files in the preview
  • maxFileCount should be set to 1
  • validateInitialCount should be set to false (not mandatory)

This is because ajax mode by nature allows you to queue files... and even if you have multiple attribute not set (i.e. false) ... that only limits file selection one at a time. User can go ahead and select one file and then once done he can go ahead and select another and so on.

If you have challenges - I can try to update over this PR - to achieve the above case with ajax.This will work similar to the avatar upload demo for configuring the plugin (which is for non-ajax mode - which exactly does what you need).

@kartik-v kartik-v force-pushed the master branch 2 times, most recently from 6b4427c to 3978315 Compare May 29, 2017 02:12
@x-yuri
Copy link
Author

x-yuri commented May 29, 2017

a lot of words down below :)

First, why do I think it doesn't suit my use case well? I don't really like how file input becomes in control of everything. Generally, you have a form, and file input is just one of the fields. With your plugin (in case of AJAX uploads), you've got file input and it's it you're supposed to provide with information which extra fields to pass. And it's it that is responsible for submitting the form. The plugin has a lot of features, great job there. But honestly, in my opinion better design would be to have one plugin for file input, one for preview area. And give back to form control over when to submit, what to submit, whatever. That would make it less monolithic, easier to change. But we're getting out of scope of this PR here. The question is, do we want to support my use case?

Speaking of which, let me describe it in more details first. I've got a list of records on a page, which are basically images with extra data (think Instagram). User can add records. For that it clicks Add button, modal appears where it fills in a form, and submits it. I can't use plain form submission, I need AJAX. Since some fields might not pass validation. And if I were to use plain form submission, I would need to redisplay the modal after showing the page again. Which is probably possible, but AJAX would make it easier.

With ajax upload mode... there is one possibility to look for a bit of enhancement - but it will be only for this scenario:

  • multiple attribute should be false - means user can always select one file
  • overwriteInitial should be set to true - to overwrite preview thumbs and not append files in the preview
  • maxFileCount should be set to 1
  • validateInitialCount should be set to false

Not sure if I follow you here. You're saying that only one specific usage scenario has room for improvement. Basically, that is the use case when user can select only one file. And here I agree.

We could probably include multiple == true + maxFileCount == 1 here, but I'd rather not. I believe only multiple == false needs special treatment. That is, I agree as well.

overwriteInitial should be set to true

Under conditions you provided, you can set initialPreview: [<img1>, <img2>]. Shouldn't we deal with it as well then?

  • maxFileCount should be set to 1

That is needed only for multiple == true, from what I can see.

  • validateInitialCount should be set to false

Why false? I'd expect to see true here. Since we're considering case when only one file can be chosen, aren't we? And being able to display two or more, even if not done by user, is probably not what is supposed to be allowed.

But upon investigation, it appeared, that validateInitialCount affects only minimum file count validation. Do you really meant validateInitialCount == false?

This is because ajax mode by nature allows you to queue files... and even if you have multiple attribute not set (i.e. false) ... that only limits file selection one at a time. User can go ahead and select one file and then once done he can go ahead and select another and so on.

I can see that with multiple == true. But when it's false (with default values for all options but uploadUrl, to be specific), I select one file, it appears in preview area, I select another one, it replaces the one in the preview area, and only one file gets uploaded when I press Upload button.

So, briefly I see the need to specialcase multiple == false case. As to the other options, I don't think they are relevant here. If developer feels like displaying two images at start... Well, maybe he has his reasons. What matters is that user can select only one file.

If you have challenges - I can try to update over this PR - to achieve the above case with ajax.

Feel free to change my PR in any way. But let us probably discuss it first, to make sure we're on the same page.

Have you checked uploading files/photos using some other common utilities - e.g. facebook in a bulk mode? Does it allow retry of the uploads that have failed OR does the user need to select and reupload again?

I just tried creating an album on Facebook. I click on Create Album button. It asks me to select files. After which they're displayed in a popup. If any of them is too big, it's shown as a box with corresponding message. I can add more photos, but I can't retry to upload photos I selected in the beginning. After I decided with which photos I post, and optionally gave them titles, I click Post button.

So the answer to your question is probably, no. It doesn't allow to retry uploads. But the workflow there is slightly different. You upload files, then you get a preview where you can amend things, and after that you can apply changes. And we were discussing doing all of that in one step. Or so I believe. You choose files, provide some extra info, then click Submit. After which form validation might fail, and you'll be presented with the same form.

@kartik-v
Copy link
Owner

kartik-v commented May 30, 2017

I don't really like how file input becomes in control of everything. With your plugin (in case of AJAX uploads), you've got file input and it's it you're supposed to provide with information which extra fields to pass. And it's it that is responsible for submitting the form.

This is more to do with the native HTML file input restrictions and nothing to do with the plugin. The native file input does not allow manipulation via javascript nor queuing and remembering multiple batches of files - nor does it allow setting values to a native file input via javascript. So if you want to use a traditional form submission - you can configure the plugin without uploadUrl ajax mode and it will allow you to use it like a native file input while you build your other input validations in your form submission.

However if you need the other features and methods to control your file uploads (as provided in ajax mode) - this will need to be a slightly different configuration for you as a developer. The plugin rather provides you a closest way to the above to have best of both in ajax mode and use your form inputs as you have and set plugin as a file uploader in parallel but just set your form input values to be returned within uploadExtraData as a JS callback. You could hide the default plugin upload button using properties and mimic another form submission button (type=button) that looks like your normal form submit button on which you can trigger the upload method of the plugin, which will upload both the files and also submit your form data via uploadExtraData.

If you have other JS or client validations that are called on your otherwise form submit - you could trigger all that validations in your filepreupload or filebatcpreupload events and return false or trigger e.preventDefault to abort the submission of the form and file upload. So technically you should be able to achieve a similar need to a form submission.

@kartik-v
Copy link
Owner

kartik-v commented May 30, 2017

  • validateInitialCount should be set to false

Yes this is not mandatory. This can be set to true.

@kartik-v
Copy link
Owner

kartik-v commented May 30, 2017

I just tried creating an album on Facebook. I click on Create Album button. It asks me to select files. After which they're displayed in a popup. If any of them is too big, it's shown as a box with corresponding message. I can add more photos, but I can't retry to upload photos I selected in the beginning. After I decided with which photos I post, and optionally gave them titles, I click Post button. So the answer to your question is probably, no. It doesn't allow to retry uploads.

Precisely my point - this is true for most HTML5 based file uploaders out there ... the only difference is how you as a developer needs to program your code for the UI logic and server logic to achieve your app's use case.

@x-yuri
Copy link
Author

x-yuri commented May 31, 2017

Do we have anything left to do in this PR?

On the UX side - there will be some issues to handle as well - because you need to properly identify the errored out thumbnails and reset their progress and relink them back to the files left over in the stack properly (assuming there are no additional new files added. If user chooses to add more files after the errors - they will add to the complication of maintaining unique preview ids and thumbnail indices to not overwrite the earlier thumbnails).

This must be for multi-upload scenario, which we doesn't deal with in this PR, I believe.

multiple == false
overwriteInitial == true
maxFileCount == 1
validateInitialCount == false

We seem to agree that only the first condition is relevant. Which is taken care of in this PR.

Anything I'm missing?

@kartik-v
Copy link
Owner

kartik-v commented May 31, 2017

No... nothing much pending.. thinking to probably to add another property to handle this scenario...

@kartik-v
Copy link
Owner

Thanks @x-yuri have merged your PR code.

@kartik-v
Copy link
Owner

Refer #1048 for a better enhancement to handle this for even multiple files.

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

Successfully merging this pull request may close these issues.

2 participants