-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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] - Color settings - Update label and show the selected color value #33973
Conversation
…nd display the selected hex color.
Hey @iamthomasbishop 👋 I made the changes to update the label and to also display the selected color hex value. Do you want a build to test it or is it ok with just the gifs from the PR description? Thanks! |
Size Change: 0 B Total Size: 1.03 MB ℹ️ View Unchanged
|
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.
Great work @geriux 👍
The code changes LGTM. While testing the app on Android I noticed that the new label is not shown when the gradient selector is shown (e.g. on buttons or cover block) and wanted to verify if this is intentional before proceeding with testing iOS.
This is out of scope but I wonder if this user request to be able to edit the hex value is a quick win in a follow up PR. Wdyt @iamthomasbishop?
Yeah if background gradients are supported for the block, you'll see the Color/Gradient selector instead of the label. I've should've mentioned that the change it's for text color settings and not backgrounds. 😅 |
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.
if background gradients are supported for the block, you'll see the Color/Gradient selector instead of the label.
Considering this I've tested the implementation on Android (Google Pixel 5, Android 12) and iOS (iPhone SE 2020, iOS 14.4.1). Everything works as expected 🎉
Thank you for this improvement @geriux 🙇
Hey @iamthomasbishop this is ready to be merged but I wanted to double-check with you if before doing it. Do you want a build to test this or is it ok if you just review the gifs from the description above? Thanks! |
Fixes wordpress-mobile/gutenberg-mobile#3766
Gutenberg Mobile PR
-> wordpress-mobile/gutenberg-mobile#3817Description
This PR updates the label of the color settings palette from
Select a color
toSelect a color above
.It also adds the functionality to display the color selected and the ability to copy it.
How has this been tested?
Paragraph
block.Screenshots
Types of changes
New change
Checklist:
*.native.js
files for terms that need renaming or removal).