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

[iOS] Implement client-defined media options. #1539

Merged
merged 5 commits into from
Nov 7, 2019

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Nov 5, 2019

Fixes #720

This PR implements the iOS Bridge code needed for Stock Photos (Pexels) and #1265

WPiOS PR: wordpress-mobile/WordPress-iOS#12858

pexels

Note:

  • The "Free Photo Library" is the same as used on Aztec.

  • I decided to make the Free Photos Library multi-selectable, even though we haven't activated this feature on iOS yet. I can keep it single selectable if we decide that this is better.

  • There are some few small changes on the bridge media picker/upload methods, so it would be good to test all options on all media related blocks. (cc @pinarol)

  • I noticed that we could improve some things in the JS side, but opted to keep this PR focused on adopting the current implementation to avoid the need of Android changes.

To test:

Image Block:

  • Add an Image block.
  • Check that the "Free Photo Library" option is there.
  • Open the Free Photo Library.
  • Choose one (or many) images.
  • Check that all images selected are added.
  • Check that you can select only images in all other options.

Video Block:

  • Add a video block.
  • Check that "Free Photo Library" is NOT in the options.
  • Check that you can only select videos in all other options.

Media&Text:

  • Add an Media&Text block.
  • Check that the "Free Photo Library" option is there.
  • Open the Free Photo Library.
  • Choose one (or many) images. (Multi-select is also possible on Android)
  • Check that all images selected are added. (Extra images will be added as Image Block)
  • Check that you can select images and videos in all other options.

cc @marecar3

Update release notes:

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

@etoledom etoledom added this to the 1.17 milestone Nov 5, 2019
@etoledom etoledom added [OS] iOS Media [Type] Enhancement Improves a current area of the editor labels Nov 5, 2019
@etoledom etoledom self-assigned this Nov 5, 2019
Comment on lines -23 to -27
public enum MediaFilter: String {
case image
case video
case audio
case other
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged MediaType with MediaFilter since we filters by Types of media. It seemed like redundant.

Comment on lines -80 to +83
func gutenbergDidRequestMedia(from source: MediaPickerSource, filter: [MediaFilter]?, allowMultipleSelection: Bool, with callback: @escaping MediaPickerDidPickMediaCallback)
func gutenbergDidRequestMedia(from source: Gutenberg.MediaSource, filter: [Gutenberg.MediaType], allowMultipleSelection: Bool, with callback: @escaping MediaPickerDidPickMediaCallback)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made filter non-optional, since a nil and empty both means no filters (use all types).

Comment on lines 48 to 50
func requestOtherMediaPickFrom(_ source: String, allowMultipleSelection: Bool, callback: @escaping RCTResponseSenderBlock) {
//TODO implement me
requestMediaPickFrom(source, filter: nil, allowMultipleSelection: allowMultipleSelection, callback: callback)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This event (requestOtherMediaPickFrom) is probably not needed? We could reuse requestMediaPickFrom.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but better check with @marecar3 if this can be changed on Android side or is there any limitation there.

Copy link
Contributor Author

@etoledom etoledom Nov 5, 2019

Choose a reason for hiding this comment

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

I don't mean to do it on this PR, but it's good to keep it in mind. There might also be a reason for it on Android?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this mentioned change here: WordPress/gutenberg#18303

@etoledom etoledom marked this pull request as ready for review November 5, 2019 12:02
@etoledom
Copy link
Contributor Author

etoledom commented Nov 6, 2019

@marecar3 - I just found a small issue on the Android implementation.

We should not show the Free Media Library option on Self Hosted site, since this is a WPCom/Jetpack-only feature.

I tested on a Self Hosted site and the images fail to upload, but curiously, the images show.

@etoledom
Copy link
Contributor Author

etoledom commented Nov 6, 2019

cc @iamthomasbishop

@marecar3
Copy link
Contributor

marecar3 commented Nov 6, 2019

@marecar3 - I just found a small issue on the Android implementation.

We should not show the Free Media Library option on Self Hosted site, since this is a WPCom/Jetpack-only feature.

I tested on a Self Hosted site and the images fail to upload, but curiously, the images show.

Thanks @etoledom, here is the quick fix PR for that case: wordpress-mobile/WordPress-Android#10743

@iamthomasbishop
Copy link
Contributor

@etoledom great catch! and thanks for the quick fix @marecar3! 😄

Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

Looking good.

@etoledom etoledom merged commit bdbe882 into develop Nov 7, 2019
@etoledom
Copy link
Contributor Author

etoledom commented Nov 7, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media [OS] iOS [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pexels and Giphy search not available form image block
4 participants