Details

Reviewers

atul
kamil
ashoat

Commits

rCOMM1d7297d5d76c: [native] fix emoji keyboard theme colors

Summary

Small follow up diff to address comments in D8310.

In this diff I removed all the instances of the panel color family and the emoji keyboard specific colors and committed to using the modal color family. Please note I tried really hard to reuse existing color variables and to not introduce any new variables for this; however, this did lead me to using some variables that didn't "fit" the proper description even though it was the best fitting color. I will highlight this using inline comments and offer an alternate solution if we think the current variable name is not fitting well.

Test Plan

Please see the screenshots below to confirm that the color themes for both dark and light mode okay. Also had @ted take a look irl

Screenshot 2023-07-06 at 4.24.11 PM.png (1×1 px, 785 KB)

Screenshot 2023-07-06 at 4.24.27 PM.png (1×1 px, 796 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Jul 6 2023, 12:17 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 6 2023, 12:17 PM

ginsu edited the summary of this revision. (Show Details)Jul 6 2023, 12:21 PM

ginsu added reviewers: atul, kamil.

fix modalKnob darkmode color

ginsu edited the summary of this revision. (Show Details)Jul 6 2023, 12:29 PM

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

ginsu attached a referenced file: F623891: Screenshot 2023-07-06 at 3.22.19 PM.png. (Show Details)

ginsu attached a referenced file: F623892: Screenshot 2023-07-06 at 3.16.01 PM.png. (Show Details)

Harbormaster completed remote builds in B20772: Diff 28451.Jul 6 2023, 12:35 PM

Harbormaster completed remote builds in B20773: Diff 28452.Jul 6 2023, 12:41 PM

ginsu requested review of this revision.Jul 6 2023, 12:41 PM

ginsu added inline comments.Jul 6 2023, 12:49 PM

native/components/emoji-keyboard.react.js
101 ↗	(On Diff #28452)	The modalSubtext variable fit the best color wise, but tbh it feels a bit weird for a background element to use a subtext variable name. (Please read the comment two below for an alternate solution)
102 ↗	(On Diff #28452)	The `modalBackgroundSecondaryLabel` variable fit the best color wise, but feels weird for a container background color to use a "label" variable name. (Please read the comment below first) If we changed instances of `modalSubtext` to `modalInputBackground` would a better solution here be to introduce a `modalInputForeground` variable?
108 ↗	(On Diff #28452)	Please read this inline comment first The `modalSubtext` variable fit the best color wise, but tbh it feels a bit weird for a background element to use a subtext variable name. I checked with our search bar element and we use `panelInputBackground` for the search bar container background color. Would introducing a `modalInputBackground` color variable be a better solution here?
native/themes/colors.js
98 ↗	(On Diff #28452)	Unfortunately, none of the current colors inside of the modal family fit well for the knob (the little element sitting on top of the emoji keyboard). Since this element is placed right on the overlay, we need this color to always contrast well with a dark background (we need to use a white color for both dark and light mode)

Adding @ashoat as a reviewer since I am introducing a new color variable

Thanks for explaining all of the thinking around colors @ginsu. I think your suggestions make sense. Feel free to re-add me if you want my perspective again

native/components/emoji-keyboard.react.js
102 ↗	(On Diff #28452)	Seems reasonable
108 ↗	(On Diff #28452)	Yes, I think that would be a better solution
native/themes/colors.js
98 ↗	(On Diff #28452)	Okay, thanks for explaining