[Mobile] - Cover block - Media editing #23280
Conversation
geriux
added
[Status] In Progress
|
Size Change: 0 B Total Size: 1.15 MB ℹ️ View Unchanged
|
geriux
force-pushed
the
rnmobile/cover-media-editing
branch
2 times, most recently
from
64840d9
to
0206627
Compare
geriux
changed the title
|
Hey @iamthomasbishop 👋 This is also ready for a design review whenever you can. Thank you! |
geriux
force-pushed
the
rnmobile/cover-media-editing
branch
from
c4b465e
to
48df5ed
Compare
There was a problem hiding this comment.
I tested this on Android (via Pixel 3a) using the steps provided and everything is working as described! I also tried leaving the editor while the upload for the newly edit image was completing, and reopening the editor shows the correct image (url is the remote one). Nice work @geriux 🎉 . Overall, code looks good, I just left a few suggestions.
|
@geriux Looks mostly good! The only thing that stands out to me is that you have to tap twice to select the background image before seeing the edit button. Would it be possible to instead show the edit button when the parent is selected? I think this might feel more intuitive. |
Thanks for the feedback @iamthomasbishop! So the thing is the Media edit button is included with the Image component, so its z-index only affects this component. In Cover, we have the overlay so if we make the button to show when the block is selected it'd show up like this:
One workaround I tried is to hide the overlay when it's selected, and only show it when the block options are shown for example:
But I'm not sure if it'd be confusing to make those opacity changes, what do you think? |
# Conflicts: # packages/block-library/src/cover/edit.native.js # packages/block-library/src/cover/style.native.scss
|
Hey @iamthomasbishop I managed to move the button 🎉
I'll work on getting some builds for you to test soon. |
|
@geriux That's great news! 🥳 Will test when a new build is available! |
|
Hey @iamthomasbishop builds are ready =) Thanks! |
|
Hey @mkevins 👋 this is ready for another review, thanks! |
There was a problem hiding this comment.
I tested a few flows, and it's looking pretty good with the new button behavior. I left a few minor comments, but overall it's looking good. 👍
I'll defer fully testing (e.g. upload failure / retry / progress, etc.) until after the other design issue(s) are resolved. Btw, the gallery image has inset border in a separate layer, so it's not being dimmed by the overlay, in case that helps with ideas.
packages/components/src/mobile/image/image-editing-button.native.js
Outdated
Show resolved
Hide resolved
|
Thank you for the feedback @iamthomasbishop !
We are adding
I think we should, I added it in the example below.
I think it'd make sense, I saw this in a mockup if I'm not mistaken, with all the Cover work with solid colors and now these changes with media editing we could easily add it.
Nice catch! I'll fix it. Here's a gif with all the changes, what do you think?
For Android, as you can see in the gif above the options have icons. Would you want to remove the icons as we did with the block options menu? e.g.
|
# Conflicts: # packages/block-library/src/cover/edit.native.js # packages/block-library/src/cover/style.native.scss
@geriux I think the
Looks good!
Looks like @antonis already added a ticket to work on that separately, which I hadn't even seen earlier -- great minds think alike? 😉
Looking great!
I think we can remove the icons, yes. This reminds me I need to write some clear guidelines for when/when not to use icons 😄 |
Sounds good! I'll add
Awesome, thanks! |
|
Hey @iamthomasbishop I've just made new builds with the latest feedback, just in case you want to give another quick check. @mkevins This is ready for another review 🙇 Thanks! |
|
This is looking good. I tested lots of flows on Android, including media upload failure, retry, cancel, as well as the editing flows like editing, changing media, removing media, etc. And all of that is working as expected. Also, the inset border is not covered by the overlay 🎉. The only potential blocker is with image preview: it seems that tapping the cover block without an image (but with a color) still tries to display the preview, even though there is no media. This results in a blank screen:
Also, since it automattically returns from the blank preview screen after a few seconds (with a toast), sometimes when you tap the back button, it leaves the editor instead. If you tap early enough, it just goes back to the editor, but if you tap just as the preview goes away on its own, the post is closed. Also, one thing to note, (though it may not be directly related this PR), it seems after adding color, there is no way to change the background to media.. or maybe I just couldn't find the option. Is this known / expected? |
Awesome! Thanks for reviewing and testing those flows!
Nice catch! I added this to check if there's an URL set. Didn't realize the the
I think this was due to passing an undefined URL to the preview so it should work as expected now.
It is known and it's being handled in another PR. Thanks for bringing it up though =D |
Thank you so much for reviewing and testing this @mkevins! Hey @iamthomasbishop 👋 Let me know if you want to review this again or if its good to merge. Thanks! |
|
@geriux Looking great to me! I noticed one tiny issue but it's already being resolved in another issue, so I think this one is good to ship! |
Gutenberg Mobile PR-> wordpress-mobile/gutenberg-mobile#2403WordPress iOS PR-> wordpress-mobile/WordPress-iOS#14420WordPress Android PR-> wordpress-mobile/WordPress-Android#12356Description
This PR adds Media editing functionality to the Cover block. With the recent changes and refactor we can reuse the same functionality as in Image and Media & Text block.
Since Cover has some overlay views, I had to extract the media editing button to be able to render it on top, so I created a new component
ImageEditingButtonwhich is the one that theImagecomponent uses but now can be imported from other components for more customization.It also adds a new prop
pickerOptionsto theMediaEditcomponent to pass extra options for the picker.How has this been tested?
Steps to test
---Edit an image
Replace an image
Full screen preview
Screenshots
Types of changes
New feature
Checklist: