Skip to content

Feat: support of multiple selection in media upload#1331

Merged
dratwas merged 25 commits intodevelopfrom
callstack/media-upload-multiple
Oct 2, 2019
Merged

Feat: support of multiple selection in media upload#1331
dratwas merged 25 commits intodevelopfrom
callstack/media-upload-multiple

Conversation

@dratwas
Copy link
Copy Markdown
Contributor

@dratwas dratwas commented Aug 27, 2019 •
edited
Loading

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 mentioned this pull request Aug 27, 2019
Merged
@dratwas dratwas force-pushed the callstack/media-upload-multiple branch from 986bf70 to 0dca9b4 Compare August 28, 2019 13:00
pinarol
pinarol reviewed Aug 29, 2019
View reviewed changes
pinarol
pinarol reviewed Aug 29, 2019
View reviewed changes
pinarol
pinarol reviewed Aug 29, 2019
View reviewed changes
@pinarol
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

dratwas commented Aug 29, 2019 •
edited
Loading

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
Copy Markdown
Contributor

pinarol commented Aug 29, 2019

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

@pinarol
Copy link
Copy Markdown
Contributor

pinarol commented Aug 29, 2019

and add tests.

really appreciate that!

@dratwas
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

pinarol commented Sep 13, 2019

Could you be able to update the branch from develop ?

@pinarol
Copy link
Copy Markdown
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
pinarol reviewed Sep 13, 2019
View reviewed changes
@dratwas dratwas mentioned this pull request Sep 16, 2019
Merged
@pinarol
Copy link
Copy Markdown
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!

pinarol
pinarol approved these changes Sep 24, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

mkevins commented Sep 27, 2019

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

@hypest hypest mentioned this pull request Sep 30, 2019
Merged
1 task
SergioEstevao
SergioEstevao reviewed Oct 1, 2019
View reviewed changes
@dratwas dratwas mentioned this pull request Oct 2, 2019
Merged
5 tasks
@mkevins mkevins mentioned this pull request Oct 2, 2019
Merged
@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

Reviewers

@SergioEstevao SergioEstevao SergioEstevao left review comments

@pinarol pinarol pinarol approved these changes

+1 more reviewer

@mkevins mkevins mkevins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.15

Development

Successfully merging this pull request may close these issues.

4 participants

@dratwas @pinarol @mkevins @SergioEstevao