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

Feat: support of multiple selection in media upload #1331

Merged
merged 25 commits into from
Oct 2, 2019

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Aug 27, 2019

In this PR I added support for multiple selections of media in MediaUpload component. It is a breaking change and apps will crash without changes from listed PRs.

Associated issue - #1298

Related gutenberg PR - WordPress/gutenberg#17397
Related Wordpres-iOS PR - wordpress-mobile/WordPress-iOS#12404
Related Wordpress-Android PR - wordpress-mobile/WordPress-Android#10556
To test:

  • set multiple prop to true in image/video block
  • run Wordpress-iOS or Wordpress-Android
  • add image/video block - multiple select should be enabled for selecting media from the device or WP Media
  • the array of selected media should be passed to the callback function

NOTE: Multiple selections of media from the device on iOS doesn't work since this functionality is implemented by the client app. Our example app from ios directory doesn't support multiple selections from the device because it uses build-in UIImagePickerController. In order to make it works in our example app, I would need to add an external library that supports multiple selections.

@dratwas dratwas force-pushed the callstack/media-upload-multiple branch from 986bf70 to 0dca9b4 Compare August 28, 2019 13:00
@pinarol
Copy link
Contributor

pinarol commented Aug 29, 2019

This is looking pretty good overall, I just wanted to point out some things early on.

So what are some pending work here?

@dratwas
Copy link
Contributor Author

dratwas commented Aug 29, 2019

Prop multiple works on iOS with these changes so basically I need to check how the cancelation mechanism works and make it works with multiple as well and add tests.

And Android implementation

@pinarol
Copy link
Contributor

pinarol commented Aug 29, 2019

thanks @dratwas ! did you have chance to try multiple uploads also?

@pinarol
Copy link
Contributor

pinarol commented Aug 29, 2019

and add tests.

really appreciate that!

@dratwas
Copy link
Contributor Author

dratwas commented Aug 30, 2019

thanks @dratwas ! did you have chance to try multiple uploads also?

AFAIK this is a responsibility of the client app. I implemented it in Wordpress-iOS here https://github.com/wordpress-mobile/WordPress-iOS/pull/12404/files#diff-0992f0530230d5beb61d0569d50f4459R40
I tried it and native side uploads media and sends all "mediaUpload" events related to it but it is not handled on JS side because at this moment JS implementation of image/edit.native.js assumes that we can get only one image. So it is prepared to support multiple media upload but needs to be handled in gallery block.

@pinarol
Copy link
Contributor

pinarol commented Aug 30, 2019

I tried it and native side uploads media and sends all "mediaUpload" events related to it

That's actually enough for this particular task thanks! Sorry If I confused you, the rest will be handled in gallery block. I just wanted to make sure we are sending the "mediaUpload" events generated by simultaneous uploads. 👍

@dratwas dratwas marked this pull request as ready for review September 10, 2019 21:35
@pinarol
Copy link
Contributor

pinarol commented Sep 13, 2019

Could you be able to update the branch from develop ?

@pinarol
Copy link
Contributor

pinarol commented Sep 13, 2019

Could you also add the associated issue to PR description?

@pinarol pinarol requested a review from mkevins September 13, 2019 16:23
@pinarol
Copy link
Contributor

pinarol commented Sep 24, 2019

Could you be able to update the branch resolve merge conflicts one more time? Sorry for being late for reviewing, I'll start after this. Thanks!

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS changes look good to me 🎉

@mkevins
Copy link
Contributor

mkevins commented Sep 27, 2019

I tested this on Android, and it is working nicely. 🎉

.gitmodules Outdated Show resolved Hide resolved
@dratwas dratwas merged commit b8e8fe1 into develop Oct 2, 2019
@pinarol pinarol added this to the 1.15 milestone Oct 2, 2019
@SergioEstevao SergioEstevao deleted the callstack/media-upload-multiple branch January 14, 2020 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants