-
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
720-Add support for giphy and pexel images #1469
720-Add support for giphy and pexel images #1469
Conversation
Added support to present giphy and pexel images
…ilable-from-image-block # Conflicts: # gutenberg
@marecar3 thanks for the ping! Happy to see that the implementation we did for Giphy Aztec is working here :)
We could always use the same icon we're using for the first option in that list (dashicons "image" icon, I believe) for the first iteration. I think we might eventually want to iterate on the bottom sheet used here for selection, but for now we can use that.
Assuming you mean the text label strings used on the bottom sheet, I think that's fine.
Let's look at what the web does and match that as a starting point in terms of alignment, and I don't know if I'd apply a background – assuming that area around the image is still tappable to select the block, that's probably fine. |
…ilable-from-image-block # Conflicts: # gutenberg
Hey @iamthomasbishop, thanks for the help 👍 This is how it looks with icons, is that ok with you? |
public static final String MEDIA_SOURCE_STOCK_MEDIA = "MEDIA_SOURCE_STOCK_MEDIA"; | ||
public static final String MEDIA_SOURCE_GIPHY_MEDIA = "MEDIA_SOURCE_GIPHY_MEDIA"; |
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 wouldn't mention "stock media" or "giphy" at the bridge level. I would say that they are all "extra options".
Ideally we would be able to add more media sources on the client app (like the iOS-only Other apps
option) without the need to modify the Bridge.
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.
Hey @etoledom that was my initial solution but I had a problem with this part of the code: https://github.com/wordpress-mobile/gutenberg-mobile/pull/1469/files#diff-37882e44592184b021430fb0aae5d21cR129-R135
For media stock, we don't have information about the upload process
which means that we need to listen for MediaSelectedCallback
( and not MediaUploadCallback
)
and for Giphy we have information about the upload process so we need to use MediaUploadCallback
Some ugly solution would be to put prefix upload
in the key so that we know which callback we need to assign to the media source.
Do you have something on your mind in this case?
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 see, that makes sense.
Just an idea: Do you think it would be possible to use a single progress-based callback (MediaUploadCallback
?), and send the progress as completed from the beginning in the Stock Photos case?
This might be a simpler workaround 🤔
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.
Hey @etoledom I managed to only use one callback for all cases (MediaUploadCallback
)
which gives us the opportunity to hide 3rd party options from RN layer.
Let me know when you do another iteration of review. Thanks.
@marecar3 The sheet is looking pretty good, but I have one suggestion that just jumped to my mind. We should probably use a Giphy logo from their design guidelines. I grabbed one of the logos from their Sketch asset library and made an SVG, which you can download here. For a preview, it looks like this (32x32, but we will scale down to match the size of our other icons, which IIRC is 24x24): |
Thanks for the suggestion @iamthomasbishop, I think it's a good one but with the current implementation, it's not so easy to set custom icons for Other resources. Let's go with default icons for this iteration and I will create a separate ticket that will bring that feature, WDYT? update: created a new ticket with some context: #1487 |
Works for me! This would be an enhancement, not a blocker 👍 |
return RNReactNativeGutenbergBridge.getOtherMediaOptions( filter, callback ); | ||
} | ||
|
||
export function requesOtherMediaPickFrom( mediaSource, multiple, 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.
Minor typo here I believe: request is missing the t
both here and in the RNReactNativeGutenbergBridge
?
Also, just noting that I think you could shorten these a bit to only be:
export const getOtherMediaOptions = RNReactNativeGutenbergBridge.getOtherMediaOptions;
export const requestOtherMediaPickFrom = RNReactNativeGutenbergBridge.requesOtherMediaPickFrom;
I personally prefer avoiding explicitly passing the arguments, but I don't think it's wrong the way you have it. In other words I'm not pushing for this change if you prefer it as-is.
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.
Hey @mchowning, thanks for the comment.
Minor typo here I believe: request is missing the t both here and in the RNReactNativeGutenbergBridge?
Fixed.
I personally prefer avoiding explicitly passing the arguments, but I don't think it's wrong the way you have it. In other words, I'm not pushing for this change if you prefer it as-is.
I just followed the current style of code in that file to avoid big differences between methods.
@@ -89,4 +89,12 @@ export function requestImageUploadCancel( mediaId ) { | |||
return RNReactNativeGutenbergBridge.requestImageUploadCancel( mediaId ); | |||
} | |||
|
|||
export function getOtherMediaOptions( filter, 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.
I don't want to block on this to get this feature out, but why are these "other"? Is there something intrinsically different in requesting an image from the device library or pexels?
I understand we might need to pass the available options through the bridge because we want to show them in the existing bottom sheet (although I think a native one is in the works), but other than that, I think the apps should define which options are available more generally.
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.
That's a good question @koke.
Main idea is to provide 3rd party media sources from native applications so that in the future we can just change native apps WPAndroid or WPiOS and everything should work (without changing JS side or bridge).
I agree that it would be easier to control which options are available from one place (JS) and not from two places (WPAndroid and WPiOS), but we can see in the future if this current solution has some big downsides and if's true, we can change approach.
maybe also @etoledom can share his opinion on this one.
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 that it would be easier to control which options are available from one place (JS) and not from two places (WPAndroid and WPiOS)
I actually think it makes more sense to define those in the apps, and avoid gutenberg making any assumptions about the available options.
As an example, the demo app still offers device library, camera, and wp media library, but only the device library works as expected. The camera option does nothing in the iOS simulator (it has no camera), and the media library option inserts a sample image/video.
It would be better if gutenberg just asked about the available options to present them in the picker, and then request the selected one. In the demo app, we could have some options like Choose from device
, Add sample image
, and Add sample video
, instead of pretending to support options that won't work.
I'm not sure how much effort this would be, but if we are already requesting the "others" through the bridge, it doesn't feel like it would be a hard thing to request all the options.
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 missed the point at first but now it's clear.
Yeah, we can do that, it does make sense, then we can easily mock resources for the demo apps also :)
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 actually think it makes more sense to define those in the apps, and avoid gutenberg making any assumptions about the available options.
Actually, the only option on gutenberg web (stand alone) is to add an image from a link. The WordPress Media Picker is some kind of "add on" iirc.
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'd suggest to implement the bridge with this new options as it is now, and on a second step, migrate the "older" options to be handled on this new way.
Hopefully we keep the testing steps and possible issues lower on this PR this way.
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.
@marecar3 I had a chance to review this, and overall it feels great. The only thing that I was going to suggest is that we focus the search input right away when launching into the FPL/Giphy search. |
Hey @iamthomasbishop, it seems that Aztec is working the same, so maybe we can raise a separate ticket, for a fix which will be presented in both Aztec and Gutenberg? |
Tested on media-text and it's working nicely 👌 |
Odd, I could've sworn we had this same discussion when building for Aztec and made it select the input first 🤔 I'll create separate issues on WPAndroid and WPiOS today. |
…ilable-from-image-block # Conflicts: # gutenberg
@iamthomasbishop I had a comment about that related to i18n wordpress-mobile/WordPress-Android#10628 (comment) |
…ilable-from-image-block # Conflicts: # gutenberg
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.
Looks good to me, just left a comment in gutenberg-bridge build.gradle, think theres a gif library we don't need without giphy, unless I'm mistaken. Otherwise 👍
@@ -117,6 +117,9 @@ dependencies { | |||
implementation "org.webkit:android-jsc:r241213" | |||
implementation project(':react-native-aztec') | |||
|
|||
// For animated GIF support | |||
implementation 'com.facebook.fresco:animated-gif:2.0.0' |
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.
Perhaps this should be removed with Giphy code?
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.
Good question @cameronvoell!
I was thinking to remove it but then I realize that we might need it as the user could already have gif
in his Wordpress Library and it would be cool that he can see animated gif
while editing the Post.
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.
Just tested this, very cool!
|
||
@objc | ||
func requestOtherMediaPickFrom(_ source: String, allowMultipleSelection: Bool, callback: @escaping RCTResponseSenderBlock) { | ||
//TODO implement me |
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.
Just curious when these will be implemented, perhaps during the iOS implementation for Pexel?
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.
Great questions @cameronvoell !
Yes, that's correct. The idea is to merge JS and Android part and then @etoledom will work on iOS side of it.
…rg-mobile into issue/AztecAndroid-IndexOutOfBoundsException-setSpan * 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (50 commits) Update issue templates Update gutenberg ref Update gutenberg ref Update issue templates Update GB ref to point to official 1.16.0 tag Update RELEASE-NOTES.txt Update JS bundles Update GB ref Update app bundles Update GB reference. [Aztec iOS]: `shouldInteractWith` will return always true to avoid crashes [Aztec iOS] Cleanup white spaces. Update bundles. Update version to 1.6.0 Updated bundles Force translation update when generating new bundles Update gutenberg ref 720-Add support for giphy and pexel images (#1469) Update bundles Update gutenberg ref ... # Conflicts: # gutenberg
Added support for giphy and pexels images
Fixes #720
WPAndroid PR: wordpress-mobile/WordPress-Android#10628
Gutenberg PR: WordPress/gutenberg#18026
Note: works only on Android (@etoledom will implement iOS part of it)
To test:
Choose from Free Photo Library
Choose from Giphy
@iamthomasbishop I have two questions regarding UI/UX:
@iamthomasbishop also about this one. There are some giphy images that can be smaller than a size of the component. I have centered them, but do we want some background on unused space?
Update release notes:
RELEASE-NOTES.txt
.