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

Don't start uploading image until user clicks submit #1306

Closed
miketaylr opened this issue Jan 27, 2017 · 10 comments
Closed

Don't start uploading image until user clicks submit #1306

miketaylr opened this issue Jan 27, 2017 · 10 comments

Comments

@miketaylr
Copy link
Member

Right now we auto-upload, the motivation being your upload is done while you're filling out details.

We should probably wait until the user hits submit before uploading to a) prevent a ton of unneeded images (people just clicking the button, checking it out) and b) not upload unneeded images if you decide to change it.

@karlcow
Copy link
Member

karlcow commented Jan 29, 2017

yes. And we need to be careful on the way we deal with the URI.

@miketaylr
Copy link
Member Author

And we need to be careful on the way we deal with the URI.

Can you clarify? "be careful" is crucial for many things that we do around public user data.

@karlcow
Copy link
Member

karlcow commented Feb 2, 2017

so currently.

  • User selects an image
  • we upload it on the server
  • we create a hash for the URI in a dated space
  • we send back this URI to the form to create the markup in markdown

It's indeed an issue for the cruft it creates on the server.

If we change the way we do it by just staging in the form and sending the image to the server, we can't have anymore the URI computed on the server side and added to the form.

What we could do though, either compute the URI on the JS side (so it is visible in the markdown textarea) and send this by the form, but it could be abused I guess. Not comfortable with this solution.

Or maybe we could show a placeholder for the image in the textarea without putting the full URI, I even wonder if we could actually put a thumbnail of the image in this textarea. Then the user presses submit it really generates the real URI on the server side and the right markdown format.
If the user doesn't, everything vanishes in between the stars 🌃

@karlcow
Copy link
Member

karlcow commented Feb 3, 2017

Another way:

  1. Select image
  2. Buffer the image locally
  3. Request a hash URI for the image on the server
  4. Use this hash URI for the markdown
  5. Once the user finally pushes submit, send the HTTP POST for the issue to GitHub and an HTTP PUT for the image at the location given by the hash URI.

@miketaylr
Copy link
Member Author

miketaylr commented Feb 3, 2017

Good thoughts. I was just thinking something simple like:

  1. select image, or add-on already put image [fill out form like normal], do nothing to image
  2. when user clicks upload "Report [as....]", do the XHR POST and request the image path, stick that in the markdown and submit the form immediately.

We could also just append the URI to bug description body after form submission (on the server side), there's no real point in adding the markdown on the client I think.

We already show a spinner, so there will be visual feedback while the upload is happening.

But I like your "Another way" option too.

@miketaylr
Copy link
Member Author

I'm afraid this might create a ton of pain for @cch5ng if I work on this before #1547 lands. Maybe we can get it done by the end of next week, or at least coordinate in-person.

@cch5ng
Copy link
Contributor

cch5ng commented Jun 22, 2017

@miketaylr thanks. I'm also assigned to #1571, should I assume that this issue (#1306) should be completed before I start making changes on #1571?

@miketaylr
Copy link
Member Author

That would probably make sense, @cch5ng. I could get this one done in a few hours after #1547 lands, so I shouldn't block you too much.

miketaylr pushed a commit that referenced this issue Jul 6, 2017
This makes things a bit more functional, using callbacks rather than awkard
doFooAndBar methods with so many side effects.

This changeset also recurses image downsampling until it's small enough.
@miketaylr miketaylr self-assigned this Jul 6, 2017
@miketaylr
Copy link
Member Author

(doing a little refactoring while i fix this, the upload stuff is particularly gross -- would be much nicer with Promises, but I don't wanna pull in a polyfill)

@miketaylr
Copy link
Member Author

Need to fix failing tests, and see what new tests I can write.

miketaylr pushed a commit that referenced this issue Jul 7, 2017
miketaylr pushed a commit that referenced this issue Jul 7, 2017
This makes things a bit more functional, using callbacks rather than awkard
doFooAndBar methods with so many side effects.

This changeset also recurses image downsampling until it's small enough.
miketaylr pushed a commit that referenced this issue Jul 7, 2017
miketaylr pushed a commit that referenced this issue Jul 7, 2017
miketaylr pushed a commit that referenced this issue Jul 11, 2017
cch5ng added a commit that referenced this issue Jul 15, 2017
also update after #1306

fix: #1571; add handler for multiple drag and drop events; update some styling

fix: #1571; update to use this.hasImage from #1603

fix: #1571; clean up addImageURL(); add _.bind() wrapper in maybeUploadImage() to fix error for this.addImageURL() call

fix: #1571; clean up code added for #1574

fix: #1571; remove obsolete handler, handleImageDrop(); for some reason after #1603 this breaks the intended functionality
cch5ng added a commit that referenced this issue Jul 19, 2017
also update after #1306

fix: #1571; add handler for multiple drag and drop events; update some styling

fix: #1571; update to use this.hasImage from #1603

fix: #1571; clean up addImageURL(); add _.bind() wrapper in maybeUploadImage() to fix error for this.addImageURL() call

fix: #1571; clean up code added for #1574

fix: #1571; remove obsolete handler, handleImageDrop(); for some reason after #1603 this breaks the intended functionality

cleanup: #1571; remove old logic to update text area

doc: #1571; update comment for dnd fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants