Skip to content

Conversation

malinajirka
Copy link
Contributor

@malinajirka malinajirka commented Jun 20, 2019

Fixes #9560

Note:
- This PR is dependant on wordpress-mobile/WordPress-FluxC-Android#1289 which needs to be merged first

Adds support for uploading post featured images asynchronously - without blocking the UI.

To test:
Initial steps for all test cases

  1. Create a draft
  2. Click on the overflow menu in the editor -> Post settings
  3. Set a featured image from a local storage

Successful upload
4. Notice the local image and a progress bar is shown during the upload
5. Notice the progress bar and it's overlay disappears when the upload finishes


Failed upload with retry
4. Notice the local image and a progress bar is shown during the upload
5. Turn on airplane mode before the upload finishes
6. Notice the "tap for options" message
7. Turn off airplane mode
8. Click on the image and choose "Retry uploading"
9. Notice the local image and a progress bar is shown during the upload
10. Notice the progress bar and it's overlay disappears when the upload finishes


Failed upload with remove
4. Notice the local image and a progress bar is shown during the upload
5. Turn on airplane mode before the upload finishes
6. Notice the "tap for options" message
7. Click on the image and choose "Remove"
8. Notice the image is removed, upload canceled and the error system notification dismissed


Cancel ongoing upload
4. Notice the local image and a progress bar is shown during the upload
5. Click on the image and choose remove
6. Notice the image is removed, upload canceled and the error system notification dismissed


Choose another image during ongoing upload
4. Notice the local image and a progress bar is shown during the upload
5. Click on the image and choose featured image option
6. Notice the previous image is removed, upload canceled and the error system notification dismissed
7. The newly picked image is being uploaded


Post List label
4. Notice the local image and a progress bar is shown during the upload
5. Go back to the post list and notice "Uploading media" label is shown


Remove all media dialog (test this for both Gutenberg and Aztec)
4. Notice the local image and a progress bar is shown during the upload
5. Turn on airplane mode
6. Go back to the editor and click on Update/Save/Publish
7. Publish confirmation dialog is shown -> click on Publish now
8. Notice another dialog is shown -> Click on "Remove all failed media"
9. The failed upload is removed and the error notification dismissed
10. Click on Update/Save/Publish again and notice the post gets published
(Note: It might be worth showing a snackbar message when we remove all the media - the flow seems a bit broken now, since nothing visible happens when you click on "Remove all failed media")

Update release notes:

  • [ x ] If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

Known issues:
- If you cancel the upload when the "error uploading media" notification is shown, the notification doesn't get canceled.

Before After
featured-image fixed-featured-image

@malinajirka malinajirka marked this pull request as ready for review June 23, 2019 12:01
@malinajirka
Copy link
Contributor Author

@osullivanchris I've added a support for adding a featured image to a post in offline (see before/after gifs). When the user picks a new image, the image is queued and uploaded later.

When the upload fails, the current solution displays a system notification, but keeps showing a circular progress bar in place of the featured image. When the user clicks on the progress bar, they can cancel the upload. I didn't want to replace the circular progress bar with "retry/cancel" buttons or similar as it might look like the user can't leave the screen. However, the current solution doesn't feel great either. Any suggestions?

cancel-featured-image

@maxme maxme self-assigned this Jun 24, 2019
@maxme
Copy link
Contributor

maxme commented Jun 24, 2019

However, the current solution doesn't feel great either.

I agree, the infinite progress bar feels wrong.

Could we show the local image instead of the progress bar? Since it's supposed to "just work" when the draft is uploaded. We could replace the "Cancel upload" popover menu by the other "Choose / Remove" menu.

@maxme
Copy link
Contributor

maxme commented Jun 24, 2019

Something wrong just happened when I was testing with an existing draft:

  • Airplane mode ON
  • Open an existing draft
  • Click on the overflow menu in the editor -> Post settings
  • Set a featured image from a local storage
  • Notice a progress bar is shown in place of the featured image
  • Go back to Post List and notice the post upload error
  • Edit that draft again
  • Turn OFF airplane mode
  • Click on the overflow menu in the editor -> Post settings
  • Notice a progress bar is shown in place of the featured image
  • Back to the editor
  • Click on Update
  • Wait until the draft is uploaded
  • Notice the featured image is not shown in the post list
  • Edit that draft again
  • Click on the overflow menu in the editor -> Post settings
  • Notice the button "Set featured image" 💥

demo video

@osullivanchris
Copy link

Hey @malinajirka I know this issue is in review so I don't want to hold it up too much. However I think there are a few clarifications needed.

  1. In the existing version, and while online, there is a blocking loading animation over the gallery (choose photo) view. In the proposed version, the loading takes place on the 'Post settings' screen. I think this is an improvement. I just want to check that you're changing that for both online and offline?
  2. Doing it on the post settings screen is good because it doesn't block the user, they can do some other things while the upload takes place. My question is whether the user can leave the 'Post settings' view while uploading is taking place? If the user can leave the post settings screen, we should have an uploading and failed state for the post card on the 'posts' screen like we do for other uploads.

Here is a quick mock for something close to what you currently have. I am using the same linear indeterminate indicator that we have on the posts screen, rather than the circular spinner. I don't think tapping the spinner to bring up a cancel option is intuitive. In this mock, the option is visible on the screen.
upload media fail

Happy to work on this further but showing you as quick as possible to get your input and make sure I understand all aspects.

@iamthomasbishop
Copy link

At first glance, I don't dislike your approach here, @osullivanchris. I do however think we might want to consider using an existing pattern – in this case, perhaps replicate what exists during upload/error states on the editor canvas itself.

Essentially, that means show the progress indicator docked to the top of the upload in-progress, and if there is an error, show the messaging/actions in an overlay. Example:

IMG_0490

@malinajirka
Copy link
Contributor Author

Thank you so much for the quick input @osullivanchris !!

In the existing version, and while online, there is a blocking loading animation over the gallery (choose photo) view. In the proposed version, the loading takes place on the 'Post settings' screen. I think this is an improvement. I just want to check that you're changing that for both online and offline?

yes, I moved the upload to the background for both online and offline.

Doing it on the post settings screen is good because it doesn't block the user, they can do some other things while the upload takes place. My question is whether the user can leave the 'Post settings' view while uploading is taking place? If the user can leave the post settings screen, we should have an uploading and failed state for the post card on the 'posts' screen like we do for other uploads.

Yes, the user can leave the screen. The error/retry state on the posts screen is without a change - the app doesn't differentiate between an image in the post content and a featured image (if either fails, the "error uploading media + retry" is shown). You can see the post list screen on the "after" gif in the description of the PR.

Here is a quick mock for something close to what you currently have. I am using the same linear indeterminate indicator that we have on the posts screen, rather than the circular spinner. I don't think tapping the spinner to bring up a cancel option is intuitive. In this mock, the option is visible on the screen.

Awesome, thanks! The only thing I'm a bit concerned about is that in offline the user will see the "Upload failed" right away -> I'm not sure they'll realize they can freely leave the screen and continue writing the post content. Wdyt?

@osullivanchris
Copy link

osullivanchris commented Jun 24, 2019

@malinajirka thanks for the clarifications that all makes sense.

Awesome, thanks! The only thing I'm a bit concerned about is that in offline the user will see the "Upload failed" right away -> I'm not sure they'll realize they can freely leave the screen and continue writing the post content. Wdyt?

I'm not too worried about that. We aren't blocking the UI with an overlay or anything. And if the user is has just done a task of trying to upload an image and it failed, they would want to know about it. Is there anything there I'm missing?

I chatted to @iamthomasbishop to get a review and you'll see his sketch above too. Thanks for bringing this pattern to my attention Thomas!

@malinajirka I think both my mock and Thomas' sketch have more or less the same functionality with different layout. We show the image the user is attempting to upload, a loading progress bar, and the ability to retry or remove if the upload failed. Does this seem do-able from your side, and is there anything you spot with either approach that would be an issue here?

If you're happy to build this, I can do a final high fidelity mock of Thomas' sketch to clarify

@malinajirka
Copy link
Contributor Author

Thank you both!

I'm not too worried about that. We aren't blocking the UI with an overlay or anything. And if the user is has just done a task of trying to upload an image and it failed, they would want to know about it.

👍

Does this seem do-able from your side, and is there anything you spot with either approach that would be an issue here?
If you're happy to build this, I can do a final high fidelity mock of Thomas' sketch to clarify

Sounds good, thanks!