[web] introduce AppThemeWrapper
ClosedPublic
Actions

Authored by ginsu on Oct 24 2023, 12:12 PM.

Details

Reviewers

atul
inka
rohan

Commits

rCOMM1250e84f80d3: [web] introduce AppThemeWrapper

Summary

This diff introduces a new component that wraps the contents of the App component, and the job of this component is to update the data attribute based on what the activeTheme of the app is. The subsequent diff will introduce the css variable list that will use this data attribute to show dark or light mode color values

Sources I used to help me with this implementation:
https://css-tricks.com/easy-dark-mode-and-multiple-color-themes-in-react/
https://stackoverflow.com/questions/56300132/how-to-override-css-prefers-color-scheme-setting

Test Plan

Please see the demo video below where the theme of the app changes based on the activeTheme redux state

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Oct 24 2023, 12:12 PM

Herald added subscribers: will, tomek, ashoat. · View Herald TranscriptOct 24 2023, 12:12 PM

ginsu edited the summary of this revision. (Show Details)Oct 24 2023, 12:20 PM

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

ginsu added reviewers: atul, inka, rohan.

ginsu edited the summary of this revision. (Show Details)Oct 24 2023, 12:28 PM

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

ginsu attached a referenced file: F835417: Screen Recording 2023-10-24 at 3.27.55 PM.mov. (Show Details)

ginsu added inline comments.Oct 24 2023, 12:31 PM

web/style.css
49 ↗	(On Diff #32352)	We need to add this here to prevent this from happening:

ginsu attached a referenced file: F835423: Screenshot 2023-10-24 at 3.30.23 PM.png. (Show Details)Oct 24 2023, 12:31 PM

Harbormaster completed remote builds in B23452: Diff 32352.Oct 24 2023, 12:52 PM

ginsu requested review of this revision.Oct 24 2023, 12:52 PM

ginsu added a child revision: D9582: [web] introduce light mode to the account settings screen.Oct 24 2023, 12:54 PM

Looks good, small question inline about defaulting to light

web/app-theme-wrapper.react.js
16 ↗	(On Diff #32352)	Does this mean we're defaulting to light mode? Personally prefer light mode, but figure we'd be defaulting to dark mode? CC @ashoat, @ted

This revision is now accepted and ready to land.Oct 24 2023, 3:20 PM

ginsu added inline comments.Oct 24 2023, 3:25 PM

web/app-theme-wrapper.react.js
16 ↗	(On Diff #32352)	probably something I should have called out... but on `native` we default to light mode as well if `activeTheme` is not defined. https://github.com/CommE2E/comm/blob/master/native/themes/colors.js#L254 Kept it the same here to be consistent across both platforms, but happy to change this if we think this is not the best option

rebase before landing

Harbormaster completed remote builds in B23471: Diff 32376.Oct 24 2023, 7:55 PM

Closed by commit rCOMM1250e84f80d3: [web] introduce AppThemeWrapper (authored by ginsu). · Explain WhyOct 24 2023, 8:09 PM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM1250e84f80d3: [web] introduce AppThemeWrapper.

We definitely should not be defaulting to a half-baked light mode implementation, but I assume this has been considered

probably something I should have called out... but on native we default to light mode as well if activeTheme is not defined.

https://github.com/CommE2E/comm/blob/master/native/themes/colors.js#L254

Kept it the same here to be consistent across both platforms, but happy to change this if we think this is not the best option

While this is true, defaultGlobalThemeInfo defaults to dark mode, which means the app defaults to dark mode. I assume we're doing the same on web?