[web] Introduce `onColorSelection` prop to `ColorSelector` component
ClosedPublic
Actions

Authored by atul on Apr 11 2022, 11:39 AM.

Details

Reviewers

Commits

Summary

Remove the internal pendingColorSelection state from ColorSelector and replace it with a onColorSelection prop that accepts a callback.

This allows us to reuse the existing possiblyChangedValue and onChangeColor state/functionality in ThreadSettingsModal.

Depends on D3684

Test Plan

Works as expected:

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

atul edited the test plan for this revision. (Show Details)Apr 11 2022, 11:39 AM

atul requested review of this revision.Apr 11 2022, 11:45 AM

• benschac requested changes to this revision.Apr 11 2022, 12:23 PM

• benschac added inline comments.

web/modals/threads/color-selector.react.js
19 ↗	(On Diff #11309)	hex values are hard to grok. Could we create an object (key: value) with a display name: Like: {'blue': '#5C9F5F'}

This revision now requires changes to proceed.Apr 11 2022, 12:23 PM

web/modals/threads/color-selector.react.js
19 ↗	(On Diff #11309)	I don't really have a preference on how it's implemented. In general, it's hard to know at a glance what value is being rendered just based on hex color alone.

atul requested review of this revision.Apr 11 2022, 12:33 PM

atul marked 2 inline comments as done.

atul added inline comments.

web/modals/threads/color-selector.react.js
19 ↗	(On Diff #11309)	These values are going to be pulled from `thread-utils.js:selectedThreadColors` in a subsequent diff, these can be considered placeholder values

tomek added inline comments.

web/modals/threads/color-selector-button.react.js
10–14 ↗	(On Diff #11309)	This component seems to be really generic and could be used in different places - which is really good. To make it fully reusable we should avoid having usage-specific names, so in this case instead of `currentThreadColor` we could say just `currentColor`. What do you think?