[RNMobile] Add disabled style to Cell component
#50665
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluiddot
added
the
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
fluiddot
force-pushed
the
rnmobile/cell-add-disabled-style
branch
from
53a9343
to
e808a49
Compare
fluiddot
commented
May 16, 2023
| @@ -251,11 +257,12 @@ class BottomSheetCell extends Component { | |||
| onBlur={ finishEditing } | |||
| onSubmitEditing={ onSubmit } | |||
| keyboardType={ this.typeToKeyboardType( type, step ) } | |||
| disabled={ disabled } | |||
There was a problem hiding this comment.
The text input is also disabled.
| @@ -225,7 +224,6 @@ class BottomSheetRangeCell extends Component { | |||
| testID={ `Slider ${ cellProps.label }` } | |||
| value={ sliderValue } | |||
| defaultValue={ defaultValue } | |||
| disabled={ disabled } | |||
There was a problem hiding this comment.
On iOS the default disabled style of the Slider already makes it transparent. So, in order to avoid making it too transparent with the Cell's disabled style, we only disable it on Android.
| @@ -61,6 +63,7 @@ export default function BottomSheetSwitchCell( props ) { | |||
| editable={ false } | |||
| value={ '' } | |||
| disabled={ disabled } | |||
| disabledStyle={ EMPTY_STYLE } | |||
There was a problem hiding this comment.
In the case of the Switch component, we don't want to make it transparent as it already provides a disabled style via the disabled prop. Hence, we pass an empty style to override it.
| disabled && disabledStyle, | ||
| styles.cellRowContainer, | ||
| ] } | ||
| pointerEvents={ disabled ? 'none' : 'auto' } |
There was a problem hiding this comment.
Although the parent component (TouchableRipple) is disabled, I noticed that some controls render input elements that also need to be disabled to prevent interaction. Instead of disabling them one by one, I simply disabled all touch events for the children when the cell is disabled.
Merged
2 tasks
|
UPDATE: I've applied another fix as shared in #50665 (comment). |
fluiddot
force-pushed
the
rnmobile/cell-add-disabled-style
branch
from
d51768d
to
4d9fd15
Compare
I noticed some issues when testing this change, so I've edited the changes and pushed another fix in 4d9fd15. Heads up that I've also updated the screenshots in the PR's description to reflect the last changes. |
fluiddot
commented
May 17, 2023
Comment on lines
+232
to
+237
| const placeholderTextColor = disabled | ||
| ? this.props.getStylesFromColorScheme( | ||
| styles.placeholderColorDisabled, | ||
| styles.placeholderColorDisabledDark | ||
| ).color | ||
| : styles.placeholderColor.color; |
There was a problem hiding this comment.
I finally calculated the placeholder text color to handle the different cases.
There was a problem hiding this comment.
This is great. I noticed this when I started my review and then noted you updated this code. :) The raw hex values stood out to me even when working on the Clear Button issue from a while ago -- thanks for addressing them here!
fluiddot
commented
May 17, 2023
| @@ -284,7 +284,15 @@ | |||
|
|
|||
| // used in both light and dark modes | |||
| .placeholderColor { | |||
| color: #87a6bc; | |||
| color: $gray; | |||
There was a problem hiding this comment.
I noticed this color is already defined globally:
|
@osullivanchris I'd appreciate your design input on these changes (I added several screenshots in the PR's description). I haven't generated an installable build, but let me know if you'd like to test them with a build and I'll create one. |
derekblank
approved these changes
May 17, 2023
Comment on lines
+232
to
+237
| const placeholderTextColor = disabled | ||
| ? this.props.getStylesFromColorScheme( | ||
| styles.placeholderColorDisabled, | ||
| styles.placeholderColorDisabledDark | ||
| ).color | ||
| : styles.placeholderColor.color; |
There was a problem hiding this comment.
This is great. I noticed this when I started my review and then noted you updated this code. :) The raw hex values stood out to me even when working on the Clear Button issue from a while ago -- thanks for addressing them here!
|
Thank @derekblank for reviewing the PR 🙇 !
I also found having the test component particularly useful when testing. In fact, I'm thinking of incorporating it into the code base for testing purposes. We could even use Storybook for this, although it might require extra work to support RN. |
|
@fluiddot I might take a closer look at this one. If you want to proceed, and if I have an improvement in mind I can let you know. A couple of things come to mind:
|
Great, thanks @osullivanchris for the feedback 🙇 ! Let me know if there's any improvement that we should make before merging the PR. These changes are blocking wordpress-mobile/gutenberg-mobile#5782, but it can wait until we provide a good solution here. Note that the disabled state is not yet used, it will be first used by the changes introduced in wordpress-mobile/gutenberg-mobile#5782.
The blue-ish is used as the placeholder color (i.e. when the text field is empty) while the grey-ish is for regular text. I understand the color difference is to differentiate when a text field is empty. Should we use the same color when the component is disabled?
Good point, I can handle that case independently to prevent making the color transparent. However, I'm wondering if the disabled state would be noticeable just by adding transparency to the right arrow. WDYT?
We could also apply a disabled style to the field name, I was tempted to make it also transparent but I felt it was too much transparency. Do you have any idea what we could do in this case? This could help the case I mentioned above about the
Good idea. This could be helpful to clarify why the field is disabled. I'll open a GitHub issue as a follow-up after we merge the PR. |
|
@osullivanchris As we discussed internally, I'm going to merge this as-is to unblock the changes from wordpress-mobile/gutenberg-mobile#5782. I'll incorporate the design changes we discussed in a separate PR. |
Seems it's not needed as the parent component (`TouchableRipple`) is already marked as disabled.
On iOS the default disabled style of the Slider already makes it transparent. So, in order to avoid make it too transparent with the Cell's disabled style, we only disable it on Android.
fluiddot
force-pushed
the
rnmobile/cell-add-disabled-style
branch
from
deb639c
to
0a8de68
Compare
|
Flaky tests detected in 0a8de68. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5044907853
|
Here is the PR with the changes. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Add disabled style to
Cellcomponent to reflect when a cell is disabled.Why?
Disabling the
Cellcomponent didn't reflect any change in the UI, which is confusing to users.How?
Change the opacity of the value component (
Text/TextInputcomponents) when theCellcomponent is disabled.Since this change might affect the different control components we use in the app, I checked the different components exported in the
@wordpress/componentspackage. The ones with ✅ are controls affected that have been verified:TextControl✅ColorControl✅RangeControl - Slider✅RangeControl - Stepper✅ToggleControl✅ (This PR reverts [RNMobile] Add disabled prop to SwitchCell component #50560 as now the disabled state is managed in theCellcomponent)BottomSheetTextControl✅BottomSheetSelectControl✅CycleSelectControl✅BaseControl(cell not used)TextareaControl(cell not used)SearchControl(cell not used)SelectControl✅FooterMessageControl(disablednot needed)QueryControls(composition of control components -SelectControl+SelectControl(TreeSelect) +RangeControl)RadioControl✅UnitControl✅SegmentedControls(cell not used)SegmentedControl(cell not used)Testing Instructions
In order to test the different control components, I created a test component for this purpose.
Click here to display patch
Testing Instructions for Keyboard
N/A
Screenshots or screencast
Click here to show Android screenshots
Light mode
Dark mode
Click here to show iOS screenshots
Light mode
Dark mode