[native] Make `ColorSelector` fully functional
ClosedPublic
Actions

Authored by • abosh on May 6 2022, 12:28 AM.

Details

Reviewers

atul

Commits

rCOMMb18185e5a9d7: [native] Make `ColorSelector` fully functional

Summary

Made ColorSelector fully functional: added onPress handling, save buttons for the thread color, and modified the ColorPickerModal height to fit the smaller size of ColorSelector (as opposed to ColorPicker).

Full context in Linear issue. Phasing out ColorPicker can be handled in a future diff. Depends on D3868.

Test Plan

Tested on iOS 15.4 and works as expected:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• abosh created this revision.May 6 2022, 12:28 AM

Herald added subscribers: • karol, • adrian, tomek, ashoat. · View Herald TranscriptMay 6 2022, 12:28 AM

• abosh edited the summary of this revision. (Show Details)May 6 2022, 12:30 AM

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

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

Harbormaster completed remote builds in B8833: Diff 12317.May 6 2022, 12:33 AM

• abosh requested review of this revision.May 6 2022, 12:33 AM

• abosh added inline comments.May 6 2022, 12:34 AM

native/chat/settings/color-picker-modal.react.js
58 ↗	(On Diff #12317)	This is the only change made in `ColorPickerModal` because it reflects the smaller modal size in the video in the Test Plan.
native/components/color-selector.react.js
37 ↗	(On Diff #12317)	Opted to make the background color of the button change to the selected color (see in the video in the Test Plan). Currently on `web`, the button is either `#404040` if disabled and purple if not. Not sure if we want to keep it that way (change on `native` to match `web` purple) or adopt this style, where the button background color changes based on the selected color. My rationale was that the color splotch might not be big enough to see the true color, whereas the button provides a larger canvas that a user can use to see the color (with some text in front of it). Plus, it's a color selector — it can be colorful! 😆

• abosh edited the summary of this revision. (Show Details)May 6 2022, 8:47 AM

• abosh added a parent revision: D3868: [native] Add active state to `ColorSelector`.

• abosh added inline comments.May 6 2022, 8:49 AM

native/chat/settings/color-picker-modal.react.js
58 ↗	(On Diff #12317)	In the final commit, this won't be there because it'll break the current `ColorPicker` (since it doesn't fit in this new modal size). It's just included so you can see the new size that's in the video in the Test Plan. Not sure if this is the best practice or not, but wanted to include it for transparency.

• karol removed a subscriber: • karol.May 6 2022, 9:18 AM

Looks good at a high level, some questions/suggestions inline

native/chat/settings/color-picker-modal.react.js
58 ↗	(On Diff #12317)	Thanks for pointing this out. We want diffs to be exactly what we plan on landing so it's best to avoid "temporary" accommodating changes. It's something you should still mention in the "Summary" section or by annotating a line(s) in the diff, but we usually don't want to leave in temporary code changes like this,
native/components/color-selector-button.react.js
10 ↗	(On Diff #12317)	We actually have a `SetState<T>` type in `lib/types/hook-types.js` that we'll want to use here. After importing that type we should have something like: +setPendingColor: SetState<string>,
19–21 ↗	(On Diff #12317)	Nice job realizing that we want to memoize this function with the `useCallback` hook
native/components/color-selector.react.js
33–38 ↗	(On Diff #12317)	I think we can simplify the logic here: const saveButtonStyle = React.useMemo( () => [ styles.saveButton, { backgroundColor: !saveButtonDisabled ? `#${pendingColor}` : "#404040" }, ], [saveButtonDisabled, pendingColor] );
40 ↗	(On Diff #12317)	Should this be `onColorSplotchSaved`?