Page MenuHomePhabricator

[web] Pull colors in `ColorSelector` from `selectedThreadColors`
ClosedPublic

Authored by atul on Apr 11 2022, 12:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 24, 4:03 AM
Unknown Object (File)
Sun, Nov 24, 3:36 AM
Unknown Object (File)
Sun, Nov 24, 1:18 AM
Unknown Object (File)
Thu, Nov 7, 9:18 AM
Unknown Object (File)
Thu, Nov 7, 9:17 AM
Unknown Object (File)
Thu, Nov 7, 9:17 AM
Unknown Object (File)
Tue, Nov 5, 8:44 PM
Unknown Object (File)
Oct 16 2024, 3:54 AM

Details

Summary

Slightly better than it was before where we were hardcoding these values twice. Will create the ColorSelectorButton components in a for loop instead of copy/pasting 10 times in subsequent diff.


Depends on D3701

Test Plan

Continues to work as before, colors show up correctly:

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul requested review of this revision.Apr 11 2022, 12:51 PM
tomek added inline comments.
lib/shared/thread-utils.js
98–100

Looking at https://stackoverflow.com/questions/5525795/does-javascript-guarantee-object-property-order/38218582#38218582 it seems like iteration order is a complicated topic. This implementation is probably fine, but I'd feel safer to sort these based on the value from the object.

This revision is now accepted and ready to land.Apr 12 2022, 3:38 AM
web/modals/threads/color-selector.react.js
21

Sorry, I should have been more direct.

example:

<ColorSelectorButton color="blue"

inside ColorSelector we access the hex value: selectedThreadColors[props.color] which would return the hex value.

selectedThreadColors[0] doesn't tell me what color this color selector is.

atul added inline comments.
lib/shared/thread-utils.js
98–100

Thanks for catching that! It looks like the color aren't appearing in the order intended. I naively assumed that the order was preserved.

It looks like one way we can approach this is something like:

const selectedThreadColors = Object.entries(selectedThreadColorsObj).sort((a, b) => a[1] - b[1]).map((each) => each[0]);

Inspired by https://stackoverflow.com/a/37607084

I'll update this diff with the change

atul added inline comments.
lib/shared/thread-utils.js
98–100

Actually, I'll make this a new diff since it seems like there's a few ways to approach this and there may be some discussion. Also to unblock and land the rest of the accepted stack.

web/modals/threads/color-selector.react.js
21

That's a fair point, but the colors were chosen arbitrarily and don't necessarily map cleanly to "English" names.

I agree that colors should usually be labeled, but it'd be difficult in this case. Open to any ideas though

lib/shared/thread-utils.js
98–100

That makes sense, thanks!