-
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
Add media edit icon to image block #1782
Add media edit icon to image block #1782
Conversation
@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. |
@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. :) |
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. |
@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:
But yeah, we can definitely change the label if we're going to implement that. :) |
Ah sorry, I missed the earlier context 😬 Thank you! |
@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. 😄 |
@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:
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. |
Moved milestone to v1.22. |
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.
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/ios/GutenbergBridgeDelegate.swift
Outdated
Show resolved
Hide resolved
@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). |
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 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).
return | ||
} | ||
|
||
let mediaToReturn = 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.
Is this necessary? Maybe we could usemedia
directly
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 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
.
@@ -378,7 +390,8 @@ public void attachToContainer(ViewGroup viewGroup, OnMediaLibraryButtonListener | |||
OnEditorAutosaveListener onEditorAutosaveListener, | |||
OnAuthHeaderRequestedListener onAuthHeaderRequestedListener, | |||
RequestExecutor fetchExecutor, | |||
OnImageFullscreenPreviewListener onImageFullscreenPreviewListener) { | |||
OnImageFullscreenPreviewListener onImageFullscreenPreviewListener, | |||
OnMediaEditorListener onMediaEditorListener) { |
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.
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).
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.
👋 @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 |
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.
LGTM!
Fixes #1583
Gutenberg
PR -> WordPress/gutenberg#19723WPiOS
related PR: wordpress-mobile/WordPress-iOS#13291WPAndroid
related PR: wordpress-mobile/WordPress-Android#11175This PR adds a button over Image blocks that shows two options:
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
totrue
:PR submission checklist:
shallow
.RELEASE-NOTES.txt
if necessary.