-
Notifications
You must be signed in to change notification settings - Fork 56
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
Conversation
public enum MediaFilter: String { | ||
case image | ||
case video | ||
case audio | ||
case other |
There was a problem hiding this comment.
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.
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) |
There was a problem hiding this comment.
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).
func requestOtherMediaPickFrom(_ source: String, allowMultipleSelection: Bool, callback: @escaping RCTResponseSenderBlock) { | ||
//TODO implement me | ||
requestMediaPickFrom(source, filter: nil, allowMultipleSelection: allowMultipleSelection, callback: callback) | ||
} |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
Thank you! |
Fixes #720
This PR implements the iOS Bridge code needed for Stock Photos (Pexels) and #1265
WPiOS
PR: wordpress-mobile/WordPress-iOS#12858Note:
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:
Video Block:
Media&Text:
cc @marecar3
Update release notes:
RELEASE-NOTES.txt
.