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

[Mobile] Cover: Adds Clear media button in block settings #23794

Merged
merged 5 commits into from
Jul 14, 2020

Conversation

antonis
Copy link
Member

@antonis antonis commented Jul 8, 2020

Fixes #23789 (parent wordpress-mobile/gutenberg-mobile#2275)

Related PRs:

Description

Adds Clear media button in block settings

How has this been tested?

  • Add a cover block
  • Press the ADD IMAGE OR VIDEO button and select a media
  • Tap on the block settings
  • Verify that there is a clear media button that removes the set image

Screenshots

With set media Without set media
Simulator Screen Shot - iPhone 11 - 2020-07-08 at 18 22 53 Simulator Screen Shot - iPhone 11 - 2020-07-08 at 18 23 14

Types of changes

Enhancement

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@antonis
Copy link
Member Author

antonis commented Jul 9, 2020

Hello @iamthomasbishop 👋 ,
I started implementing this following your second suggestion.

The other place we might want to consider showing this option is in the block settings sheet. We obviously don't have any media-related settings in the sheet yet, but I think there was some discussion about adding focal point and other items soon, so I would put this "clear media" button nearby those items (similar to web).

Please let me know if you agree with this or if I should add this to the menu appearing on long-press.

@antonis antonis marked this pull request as ready for review July 9, 2020 15:44
@geriux geriux self-requested a review July 9, 2020 15:52
@geriux geriux added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Jul 9, 2020
@iamthomasbishop
Copy link

iamthomasbishop commented Jul 10, 2020

@antonis looks good, just one request - can we align the "clear media" text label to the left of the cell? Normally I'd center the button like that, but it looks a bit "lonely" in this context. 😀

image

@antonis
Copy link
Member Author

antonis commented Jul 10, 2020

Thank you for the prompt feedback @iamthomasbishop. I updated the PR

@geriux
Copy link
Member

geriux commented Jul 13, 2020

Hey @antonis 👋

While testing I think I found a small bug, when I tap on Clear media it clears the media successfully but the bottom sheet remains open:

@antonis
Copy link
Member Author

antonis commented Jul 13, 2020

Thank you for your feedback @geriux 🙇
I agree with your suggestions. I submitted the changes along with the bug fix leaving the bottom sheet open.

Copy link
Member

@geriux geriux left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work @antonis! 🚀 Tested it on both iOS and Android.

@antonis antonis self-assigned this Jul 14, 2020
@antonis antonis linked an issue Jul 14, 2020 that may be closed by this pull request
@antonis antonis added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement. labels Jul 14, 2020
@antonis antonis merged commit 0e2040f into WordPress:master Jul 14, 2020
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mobile] Cover: Allow removing the selected media
3 participants