Details

Reviewers

Commits

rCOMM025e2fdf12b3: [native] Add active state to `ColorSelector`

Summary

Part of ongoing redesign of ColorPicker component on native to match the ColorSelector component on web. Depends on D3798.

Adds active state to ColorSelector by adding styling and a checkmark to the ColorSelectorButton that is currently selected as the thread color.

Will add onPress handling and updating the thread color in a later diff, as outlined in the plan on this Linear issue.

Test Plan

Tested on iOS 15.4 and works as expected:

Diff Detail

Repository

rCOMM Comm

Branch

active-may5th (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• abosh created this revision.Apr 27 2022, 7:09 PM

Herald added subscribers: • karol, • adrian, tomek, ashoat. · View Herald TranscriptApr 27 2022, 7:09 PM

• abosh edited the summary of this revision. (Show Details)Apr 27 2022, 7:10 PM

• abosh edited the test plan for this revision. (Show Details)

• abosh added 1 blocking reviewer(s): atul.

Harbormaster completed remote builds in B8632: Diff 12022.Apr 27 2022, 7:14 PM

• abosh requested review of this revision.Apr 27 2022, 7:14 PM

• abosh added inline comments.Apr 27 2022, 7:18 PM

native/components/color-selector-button.react.js
45 ↗	(On Diff #12022)	Why did `ColorSelectorButton` shift from returning a `View` to returning a `TouchableOpacity`? Styling a `TouchableOpacity` as a color splotch that should be selected is easier than styling a view because, per the React Native docs, a `TouchableOpacity` is "a wrapper for making views respond properly to touches." This adds lots of helpful functionality that a generic `View` does not provide (like opacity changes on press down).

Couple of changes and I think it should be good

As an aside, did you find designs with the glow/checkmark on Figma? I did a quick scan and couldn't find anything, would be handy if you could link to it.

If there aren't designs for native, we should probably stick with the designs from web?

native/components/color-selector-button.react.js
9 ↗	(On Diff #12022)	Good catch making this readonly
19–27 ↗	(On Diff #12022)	https://www.notion.so/commapp/Always-exit-early-9df0993fda5e41fd9c6e5e9a2248436d
21–22 ↗	(On Diff #12022)	We can move these (along with `shadowOpacity` and `shadowRadius` to `StyleSheet.create(` and create a style named `activeButton` or something. Then we can move `shadowColor` to the `selectedButtonStyle` `useMemo` and set it conditionally based on `isSelected`. That should reduce some clutter "inside the component" and push it to the outer `styles` object.
38 ↗	(On Diff #12022)	Maybe `selectedButtonIndicator`? I usually associate "content" with like images/text/etc... not really with a checkmark?
native/components/color-selector.react.js
17–23 ↗	(On Diff #12022)	This should be wrapped in `React.useMemo` as it is in `web/.../color-selector.react.js` Did you run into any issues if/when you tried this?

This revision now requires changes to proceed.May 2 2022, 7:05 AM

Addressed inline feedback and updated styles by removing glow. For now, it's basically mimicking the styles on web (see attached image) except the styles on web use a hover property which can't really be done on native. Instead, the checkmark is what indicates to the user what the current color is. Didn't seem to find a Figma design for native, so that might have to be updated or decided upon.

Since the glow is no longer needed, also removed the selectedButtonStyle useMemo and added the justifyContent and alignItems styling to all buttons. This way, if any become selected, the only extra useMemo call is for the checkmark.

Harbormaster completed remote builds in B8712: Diff 12133.May 2 2022, 8:28 AM

For now, it's basically mimicking the styles on web (see attached image) except the styles on web use a hover property which can't really be done on native. Instead, the checkmark is what indicates to the user what the current color is.

We can still use the translucent outer ring to indicate that a color is selected, right?

Added translucent outer ring and removed checkmark to match web design.

Harbormaster completed remote builds in B8742: Diff 12181.May 3 2022, 7:33 PM

Looks close! I think this diff should be good once the feedback about outerRingStyle is addressed.

native/components/color-selector-button.react.js
20–25 ↗	(On Diff #12181)	Instead of this, we should create a new style called `outerRingActive` or `outerRingSelected` within `StyleSheet.create(` that has `backgroundColor` set to "#404040" Then we could just do: <View style={isSelected ? outerRingSelected : outerRing}> or something and avoid having to create an object