[web] Introduce `ColorSelector` component with basic styling
ClosedPublic
Actions

Authored by atul on Apr 7 2022, 3:43 PM.

Details

Reviewers

tomek
• jacek
• benschac

Commits

rCOMMc673fd7cc389: [web] Introduce `ColorSelector` component with basic styling

Summary

Introduce ColorSelector component with basic layout/styling.

Note: The actual functionality will be introduced in subsequent diffs.

Depends on D3660

Test Plan

Looks as expected:

Diff Detail

Repository

rCOMM Comm

Branch

april7clean (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 7 2022, 3:43 PM

Herald added subscribers: • abosh, • benschac, • karol, • adrian. · View Herald TranscriptApr 7 2022, 3:43 PM

atul edited the test plan for this revision. (Show Details)Apr 7 2022, 3:44 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 7 2022, 3:45 PM

Harbormaster failed remote builds in B7999: Diff 11204!

Harbormaster completed remote builds in B7999: Diff 11204.Apr 7 2022, 4:09 PM

atul requested review of this revision.Apr 7 2022, 4:09 PM

atul added inline comments.Apr 7 2022, 4:26 PM

web/modals/threads/color-selector.react.js
12	These hardcoded placeholder values are from `lib/shared/thread-utils/selectedThreadColors`

• jacek added inline comments.Apr 8 2022, 3:06 AM

web/modals/threads/color-selector.react.js
12	Maybe it's better not to hardcode color values and instead export the colors from `thread-utils` and map them into components? Wouldn't it be better to use `flex-wrap` to wrap components to create rows that way? If new colors were added or removed in the future (or the size of component changes), the component could display as many rows as it needs. But it's just an idea - maybe the current approach is better

tomek requested changes to this revision.Apr 8 2022, 3:13 AM

tomek added inline comments.

web/modals/threads/color-selector.react.js
11	We could achieve this with `flex-wrap: wrap`
12	It's not ideal that we need to hardcode these values. Can we just iterate that table here? (we can change the order of elements in it)

This revision now requires changes to proceed.Apr 8 2022, 3:13 AM

tomek added inline comments.Apr 8 2022, 3:15 AM

web/modals/threads/color-selector.react.js
12	@def-au1t you were slightly faster with the same ideas as mine - Nice!

The feedback about pulling the colors from thread-utils and using flex-wrap for the layout makes sense!

I was going to include some of those changes in this diff, but it got kind of large. I'll address those two points in subsequent diffs.

This diff was mostly just to get the design/layout right

Add me back if you need me

atul requested review of this revision.Apr 10 2022, 7:18 PM

In D3662#100907, @atul wrote:

The feedback about pulling the colors from thread-utils and using flex-wrap for the layout makes sense!

I was going to include some of those changes in this diff, but it got kind of large. I'll address those two points in subsequent diffs.

This diff was mostly just to get the design/layout right

Ok, that makes sense. Do you plan to create these diffs soon or are you going to create tasks for them?

This revision is now accepted and ready to land.Apr 11 2022, 4:23 AM

Ok, that makes sense. Do you plan to create these diffs soon or are you going to create tasks for them?

Yeah it's what I'm currently working on. I don't want to clutter up Linear with a new task, but I can add a note to the existing ThreadSettingsModal one.

Pulled colors from thread-utils in a local commit and everything worked without issue.

Little less confident on using flex-wrap and/or if it's necessary at this point when we have a fixed number of colors? I think it would be nice to have a ColorSelector component that "scales" to future needs, but also I don't want to spend too much time trying to figure out that "correct" approach. If it's quick, then I'll definitely make the change... but don't want to spend hours on something that would make no difference to the user in the immediate/medium term.

• benschac added inline comments.Apr 11 2022, 8:32 AM

web/modals/threads/color-selector.css
9	nit: flex-direction is row by default. We don't need it explicitly defined.
web/modals/threads/color-selector.react.js
23	nit: could just make this prop optional, then active could be implicitly false by default and we wouldn't need to explicitly define them a bunch of times here.

atul marked an inline comment as done.Apr 11 2022, 9:10 AM

atul added inline comments.

web/modals/threads/color-selector.css
9	Personally prefer being explicit about it and don't think it hurts to leave in, but can remove if there's a strong opinion
web/modals/threads/color-selector.react.js
23	this prop gets removed altogether in later diff