-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Add Media Button when only colour is selected #23878
[Mobile] Cover: Add Media Button when only colour is selected #23878
Conversation
fa81921
to
6783f1b
Compare
…th_custom_color # Conflicts: # packages/block-library/src/cover/style.native.scss # packages/components/src/color-palette/index.native.js
Thank you for the feedback @iamthomasbishop :) I updated the button design and changed the text label.
|
Perfect. Thank you @antonis ! |
7e52411
to
1c53bdd
Compare
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 is looking pretty good. I tested on Android, and the logic is working as expected. There is one issue with the button size / container size that makes tapping difficult. It seems that the tappable area is small, and offset from the button itself:
I inspected the layout and it seems that the views are rendered outside their container (so they don't capture the event). E.g. the red rectangle is the outer view and the blue one is the child view:
It seems that aside from being offset, the size of the touchable is that of the inner view (inner view, e.g. icon, is the red rectangle):
I tried locally changing the order (so TouchableWithoutFeedback
wraps View
), but the problem still persisted, so perhaps a change is needed elsewhere, either instead of or in addition to that.
{ renderBackground( getMediaOptions ) } | ||
{ isParentSelected && | ||
hasOnlyColorBackground && | ||
addMediaButton( openMediaOptionsRef.current ) } |
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.
Do we need to pass this here? Maybe we can use it directly in addMediaButton
. Wdyt?
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.
Hello @mkevins 👋
thank you for your feedback, I really appreciate it :)
You are right, the parameter can be accessed directly. I updated the PR changing this and fixing the tappable area.
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 and it's working correctly now. Nice work @antonis ! 🎉
I left one minor suggestion (as a patch, since the GitHub diff context couldn't capture it as a "one-click-suggestion"), not a blocker though.
Fixes #23877 (parent wordpress-mobile/gutenberg-mobile#2275)
Related PRs:
This PR depends on #23870
gutenberg-mobile
Cover: Add Background Media button when only background colour is set wordpress-mobile/gutenberg-mobile#2513Description
An image icon button has been added at the top-left of the component. When pressed the add media options are presented.
How has this been tested?
Screenshots
Types of changes
Enhancement
Checklist: