-
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
[RNMobile] Color settings #21326
[RNMobile] Color settings #21326
Conversation
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.
Hey @lukewalczak 👋 left a few comments but nothing really important. I'm going to test on both iOS and Android to see if I spot anything.
packages/block-editor/src/components/block-settings/container.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/segmented-control/index.native.js
Outdated
Show resolved
Hide resolved
@@ -50,7 +52,7 @@ const cleanEmptyObject = ( object ) => { | |||
: cleanedNestedObjects; | |||
}; | |||
|
|||
function ColorPanel( { settings, clientId, enableContrastChecking = true } ) { | |||
function ColorPanel( { settings, clientId, enableContrastChecking = isWebPlatform } ) { |
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.
Hmm, I'm wondering if we could change it a bit to block the contrast checking for mobile even if it is set to true
in props?
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 can add check isWebPlatform
before each enableContrastCheking
.
packages/block-editor/src/components/colors-gradients/panel-color-gradient-settings.native.js
Show resolved
Hide resolved
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! ✅ Great work on this @lukewalczak !!
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.
Awesome work @lukewalczak 🎉
It brings me such great joy to see this work merged — amazing work @lukewalczak! 👏 |
I stumbled upon this PR while trying to move It looks like colors are handled differently on mobile. What's the reason for using conditions like: __experimentalColor: Platform.OS === 'web', Would it be okay if we would not run the code that uses this flag on mobile instead? It would be still valuable to share the same configuration between mobile and web. You need to keep in mind that this is something that site owners or plugin authors can modify to customize their experience. |
Currently, on mobile, we are supporting colors only in a few blocks such as The second options is to create separate file dedicated for the mobile platform ( |
@lukewalczak, thank you for the response. Would something like #22502 work as intended with the current state of support for colors in the mobile app? |
Description
Fixes wordpress-mobile/gutenberg-mobile#1692
Ref to testing wp-ios: wordpress-mobile/WordPress-iOS#13690
Ref to testing wp-android: wordpress-mobile/WordPress-Android#11571
Ref to gutenberg-mobile: wordpress-mobile/gutenberg-mobile#2096
That PR introduces the color settings for mobile component (in that PR color settings are connected to button component).
Color Settings allow to choose:
How has this been tested?
Check them there
Screenshots
Quick video preseting color settings functionality.
Some screenshots
Types of changes
Checklist: