[web] Replace `ColorPicker` with `ColorSelector` in `ThreadSettingsModal`
ClosedPublic
Actions

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

Details

Reviewers

• jacek
tomek

Commits

rCOMM5eaae2046b7a: [web] Replace `ColorPicker` with `ColorSelector` in `ThreadSettingsModal`

Summary

We're ready to swap out ColorPicker with ColorSelector.

Leaving ColorPicker in the codebase for now even though it's not being used anywhere... will remove it in a bit.

Depends on D3697

Test Plan

Looks and works as expected:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Apr 11 2022, 11:53 AM

Herald added subscribers: • abosh, • benschac, • karol and 2 others. · View Herald TranscriptApr 11 2022, 11:53 AM

atul added inline comments.Apr 11 2022, 11:56 AM

web/modals/threads/color-selector-button.react.js
16 ↗	(On Diff #11312)	This is a hack for now. We were previously just comparing strings, but the `currentThreadColor` we get from the server is lowercase whereas the colors we specified in `ColorSelector` are capitalized. Next diffs: Add `tinycolor2` to `web/package.json` (same version as in lib/native of course) Use `tinycolor2.equals(a, b)` to compare the two colors. This is a little less flaky than `.toLowerCase()` since it also accounts for the `#` prefix being included/excluded/etc.

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

Harbormaster completed remote builds in B8087: Diff 11312.Apr 11 2022, 12:00 PM

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

atul edited the summary of this revision. (Show Details)Apr 11 2022, 12:15 PM

atul added a parent revision: D3697: [web] Introduce `onColorSelection` prop to `ColorSelector` component.

atul added a child revision: D3700: [web] Add `tinycolor2` to `web/package.json`.Apr 11 2022, 2:58 PM

Looking at the video it feels like it would be better if hover and selected states of color button were different. Now for me it feels a bit laggy - maybe because the previous selection has longer animation duration. What do you think? (we can also keep the current solution, because it doesn't matter that much and will take some time to fix)

This revision is now accepted and ready to land.Apr 12 2022, 3:30 AM