Add media edit icon to image block by leandroalonso · Pull Request #1782 · wordpress-mobile/gutenberg-mobile

leandroalonso · 2020-01-17T15:03:57Z

Gutenberg PR -> WordPress/gutenberg#19723

WPiOS related PR: wordpress-mobile/WordPress-iOS#13291
WPAndroid related PR: wordpress-mobile/WordPress-Android#11175

This PR adds a button over Image blocks that shows two options:

Edit: shows the Media Editor so the user can edit the image
Replace: to replace the media

As for now, the button is enabled to show only in iOS, since the feature is in the first stages of development in Android.

To test

In order for this PR to work you'll have to checkout my fork-branch (I don't have permissions on the main gutenberg repo). You can use a tool like hub to make that easier. Eg.: hub pr checkout 19723

Then, for both Android and iOS (to enable in Android just change showMediaEditorButton to true:

Check that the button appears only for Images
Check that the button appears only for images that are uploaded (and not being uploaded)
Tap the button and checks that a Picker appears

PR submission checklist:

I have considered adding unit tests where possible. — Refer to Gutenberg's parent PR. I had some issues using shallow.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

leandroalonso · 2020-01-17T15:08:04Z

@iamthomasbishop @mbshakti I implemented the button based on the last version that was shared:

The button is slightly bigger because I wanted to respect the minimum 44/44pt touch area. Let me know if that's a problem.

Also, I wasn't able to implement the "Delete" option. It seems to me that this is a little bit complicated, but I need help from @etoledom here. Maybe it is just my lack of experience in React talking.

leandroalonso · 2020-01-17T16:44:31Z

@hypest we talked about not including a conditional platform to show this button.

However, we just start Android and it's way behind iOS. I've added the conditional and I can promise I'll watch that code and remove it once Android is ready. 🤞

Are you ok with that? Let me know if you have any other suggestions. :)

iamthomasbishop · 2020-01-17T16:45:00Z

Looking pretty solid! I one request: can we change the “delete” label to “remove”? I think this makes more sense because you’re not actually deleting the image, rather removing from the document.

leandroalonso · 2020-01-17T16:47:57Z

@iamthomasbishop the image in the PR description is actually what was implemented, for now, we don't have the "Delete" option.

Quoting my last comment:

Also, I wasn't able to implement the "Delete" option. It seems to me that this is a little bit complicated, but I need help from @etoledom here. Maybe it is just my lack of experience in React talking.

But yeah, we can definitely change the label if we're going to implement that. :)

iamthomasbishop · 2020-01-17T16:49:16Z

Ah sorry, I missed the earlier context 😬 Thank you!

mchowning · 2020-01-17T19:46:40Z

@iamthomasbishop , is there a plan to consolidate all of these "edit" buttons. With this change we have three 😱 ? Perhaps the intention is for this button to replace the swap image button in the toolbar since they seem to duplicate each other? Even if that's true, I think it will still be a bit confusing to figure out which of the remaining two remaining "edit" buttons are used for a particular "edit". Just my $.02. 😄

iamthomasbishop · 2020-01-20T14:54:53Z

is there a plan to consolidate all of these "edit" buttons. With this change we have three 😱 ?

@mchowning My thought is that the most obvious place to put a "replace" entry point is on the image itself. The image in the toolbar is not at all obvious, so I'd like to do one of the following:

Change the icon to a text label of Replace (as core recently changed to, IIRC). This would require us to change other media blocks that offer replace functionality.
Remove it once we have this menu button on the image itself.

It's worth noting that the button in the toolbar is always visible, whereas there is always a chance that the others aren't. Having a button in the toolbar is generally valuable real estate for primary actions because of this. The replace action specifically is just not a particularly obvious representation/icon, imo. 😄

The other one is the options in the settings sheet. I'm not as worried about this because it's "hidden" UI until the user needs it, thus less of a conflict.

etoledom · 2020-01-20T22:45:29Z

Moved milestone to v1.22.
Please feel free to re-target if you consider this PR must be shipped together with v1.21.

Let's make CI happy again

etoledom

Thank you so much @leandroalonso for this solid PR, its looking great! 🎉

I added a few comments but there's nothing really really important.

Also, for the Android side, I'd like to ask some else to take a look at it (@marecar3 or @mchowning perhaps?)

react-native-gutenberg-bridge/index.js

react-native-gutenberg-bridge/ios/GutenbergBridgeDelegate.swift

ios/gutenberg/GutenbergViewController.swift

react-native-gutenberg-bridge/ios/GutenbergBridgeDelegate.swift

leandroalonso · 2020-01-23T16:23:13Z

@etoledom I guess I addressed all the necessary changes. Let me know if there's anything else.

As for the PR in WPiOS, I'll create one that will both implement this method and also provide the Media Editor (it wouldn't make sense to have just an option that does nothing).

etoledom

Looks great!

Just added a tinny comment, nothing really important.

I still would like to have some 👀 on the Android bridge code. @mchowning maybe since you already have commented here? :)

Is the WPiOS PR Up with the bridge method implementation (even if it's an empty one).

etoledom · 2020-01-27T09:48:39Z

react-native-gutenberg-bridge/ios/RNReactNativeGutenbergBridge.swift

+                    return
+                }
+
+                let mediaToReturn = media


Is this necessary? Maybe we could usemedia directly

mchowning

This looks pretty good to me. As noted below, the only thing I think we need is a PR to WPAndroid that insures we can still build WPAndroid when BUILD_GUTENBERG_FROM_SOURCE set to true.

mchowning · 2020-01-27T20:47:19Z

...nberg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java

                                  RequestExecutor fetchExecutor,
-                                  OnImageFullscreenPreviewListener onImageFullscreenPreviewListener) {
+                                  OnImageFullscreenPreviewListener onImageFullscreenPreviewListener,
+                                  OnMediaEditorListener onMediaEditorListener) {


We also need a PR to WPAndroid that adds this new parameter where GutenbergContainerFragment calls this method. Otherwise, merging this will break building Gutenberg from source on WPAndroid (i.e., building with wp.BUILD_GUTENBERG_FROM_SOURCE=true added to WPAndroid's gradle.properties file).

Done in wordpress-mobile/WordPress-Android#11175

mchowning · 2020-01-27T20:52:00Z

👋 @leandroalonso ! Nice work on this!

We have recently merged a fix for our e2e tests that should make them more stable. So you may want to merge/rebase the latest from develop into your PR to increase the likelihood of getting green builds on CI. 😄

mchowning

LGTM!

leandroalonso added 3 commits January 17, 2020 11:25

Add MediaEditor as a media source

24c44bd

Adds Media Editor call in iOS and Android projects

5cc7b7c

Update submobule commit

723422e

leandroalonso added Media [Status] Needs Design Review Needs design review or sign-off before shipping [Status] DO NOT MERGE Do not merge this PR labels Jan 17, 2020

leandroalonso added this to the 1.21 milestone Jan 17, 2020

leandroalonso requested review from SergioEstevao and etoledom January 17, 2020 15:03

Update submodule commit

fec309e

leandroalonso added 4 commits January 17, 2020 16:58

Update submodule reference

a19bc00

Implements requestMediaEditor bridge method for iOS

4afaf29

Adds requestMediaEditor bridge method for Android

deef065

Update gutenberg submodule

6011985

etoledom modified the milestones: 1.21, 1.22 Jan 20, 2020

leandroalonso added 5 commits January 21, 2020 15:58

Merge branch 'develop' into issue/1583_add_overflow_menu_button_to_image

7702822

Point gutenberg to my fork

da9adaa

Let's make CI happy again

Fix wrong merge change

7dd6e18

Update subproject commit

e91b9f3

Remove dash

4928edf

etoledom reviewed Jan 22, 2020

View reviewed changes

leandroalonso added 6 commits January 22, 2020 15:43

Only display the button when in development

5ca3c27

Remove Media Editor as an internal source

4535a1b

Remove media editor as a media source

1e3bd5f

Update gutenberg commit

29fcef5

Update gutenberg commit

6228bb5

Update gutenberg

cce42b1

etoledom self-requested a review January 23, 2020 14:11

Calls the callback with a different image to check if it works

2257b1c

etoledom approved these changes Jan 27, 2020

View reviewed changes

leandroalonso added 2 commits January 27, 2020 10:09

Merge branch 'develop' into issue/1583_add_overflow_menu_button_to_image

dd2e639

Update gutenberg

063ec30

mchowning suggested changes Jan 27, 2020

View reviewed changes

leandroalonso added 2 commits January 27, 2020 18:03

Merge branch 'develop' into issue/1583_add_overflow_menu_button_to_image

d268e4e

Update gutenberg

bed0461

leandroalonso removed [Status] DO NOT MERGE Do not merge this PR [Status] Needs Design Review Needs design review or sign-off before shipping labels Jan 28, 2020

leandroalonso added 2 commits January 28, 2020 09:50

Merge branch 'develop' into issue/1583_add_overflow_menu_button_to_image

7dd42a9

Update gutenberg

999f0df

mchowning approved these changes Jan 28, 2020

View reviewed changes

leandroalonso merged commit 7963eee into develop Jan 28, 2020

leandroalonso deleted the issue/1583_add_overflow_menu_button_to_image branch January 28, 2020 16:51

This was referenced Feb 3, 2020

Release v1.22.0 #1849

Merged

Gutenberg: update to v1.22.0 wordpress-mobile/WordPress-iOS#13353

Merged

Gutenberg: update to v1.22.0 wordpress-mobile/WordPress-Android#11224

Merged

ashiagr mentioned this pull request Mar 26, 2020

[RNMobile] Enable edit button over Image blocks for Android WordPress/gutenberg#21165

Merged

6 tasks

Conversation

leandroalonso commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test

Uh oh!

leandroalonso commented Jan 17, 2020

Uh oh!

leandroalonso commented Jan 17, 2020

Uh oh!

iamthomasbishop commented Jan 17, 2020

Uh oh!

leandroalonso commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamthomasbishop commented Jan 17, 2020

Uh oh!

mchowning commented Jan 17, 2020

Uh oh!

iamthomasbishop commented Jan 20, 2020

Uh oh!

etoledom commented Jan 20, 2020

Uh oh!

etoledom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leandroalonso commented Jan 23, 2020

Uh oh!

etoledom left a comment

Choose a reason for hiding this comment

Uh oh!

etoledom Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

mchowning left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mchowning Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

leandroalonso Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

mchowning commented Jan 27, 2020

Uh oh!

mchowning left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leandroalonso commented Jan 17, 2020 •

edited

Loading

leandroalonso commented Jan 17, 2020 •

edited

Loading

mchowning left a comment •

edited

Loading