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

Simplify bridge requestMediaPick methods #1547

Merged

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Nov 6, 2019

This PR simplifies and merges all requestMediaPick... methods into one requestMediaPick, including the new requestOtherMediaPickFrom method.

This allow us to handle media pick request from any source in the same way, and giving the same parameters.

Passing the filter parameter to requestOtherMediaPickFrom was needed for wordpress-mobile/WordPress-iOS#12883 to work properly, so I decided to do this extra enhancement.

This will probably break the WPAndroid integration of Free Photo Library. (cc @marecar3 )

This is also a good step forward to move all media source definitions to the client side.

gutenberg side PR: WordPress/gutenberg#18303
WPiOS PR: wordpress-mobile/WordPress-iOS#12883

cc @pinarol

To test:

  • Please follow the steps on the WPiOS PR.

Update release notes:

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

@etoledom etoledom added [Type] Enhancement Improves a current area of the editor Media labels Nov 6, 2019
@etoledom etoledom added this to the 1.17 milestone Nov 6, 2019
@etoledom etoledom self-assigned this Nov 6, 2019
@etoledom etoledom marked this pull request as ready for review November 6, 2019 10:36
@etoledom etoledom requested a review from marecar3 November 6, 2019 10:36
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.

Love this change!

@etoledom etoledom force-pushed the issue/remove-extra-media-pick-methods-from-bridge branch from 587a51b to af001b9 Compare November 8, 2019 07:52
@etoledom etoledom changed the base branch from issue/free-medial-library-ios to develop November 8, 2019 07:55
@etoledom etoledom changed the base branch from develop to release/1.17 November 13, 2019 12:17
Comment on lines +13 to +17
export const mediaSources = {
deviceLibrary: 'DEVICE_MEDIA_LIBRARY',
deviceCamera: 'DEVICE_CAMERA',
siteMediaLibrary: 'SITE_MEDIA_LIBRARY',
};
Copy link
Contributor Author

@etoledom etoledom Nov 13, 2019

Choose a reason for hiding this comment

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

@SergioEstevao - I decided to declare the media source constants bundled in an object.
Anyway, at some point we will probably move all this to the parent apps.

@SergioEstevao
Copy link
Contributor

@etoledom tested it again in iOS and Android all looks good!

Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

Tested all scenarios on Android device, nice work!
LGTM!

@etoledom etoledom merged commit c90c5b5 into release/1.17 Nov 14, 2019
@etoledom etoledom deleted the issue/remove-extra-media-pick-methods-from-bridge branch November 14, 2019 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Media [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants