Automatically follow user's OS-level dark/light preference. by Gautam-Arora24 · Pull Request #4555 · zulip/zulip-mobile

Gautam-Arora24 · 2021-03-21T18:26:47Z

Previous Functionality

Earlier, there was just a switch in the SettingsScreen to enable or disable the Night mode.

Screen.Recording.2021-03-22.at.12.26.33.AM.mov

Changes Done

This PR creates a new component called ThemeScreen and provides 3 options to a user for the theme in the Zulip mobile app.

Dark (ignore OS-level setting)
Light (ignore OS-level setting)
System Default (makes use of OS-level setting)

Screen-Recording-2021-03-21-at-12.31.42-PM.mov

PS - One useful shortcut to change the Appearance on iOS simulator is Cmd + Shift + A
Fixes: #4009

chrisbobbe

Thanks, @Gautam-Arora24! In particular, I appreciate your attention to the commit message! 🙂 See some comments below.

Also, there's a point mentioned at #4009 (comment) about changing the name for the 'default' theme to something else, like 'light', and Greg also mentions at #4009 (comment) that the naming for the dark mode/theme should use 'dark' instead of 'night'. Let's do those renames in a separate NFC commit or two; could go before or after the functional changes of the PR. Or it could go in a separate PR as a followup, if you'd prefer to focus on the more exciting changes in this PR (we should just be sure not to forget about it). 🙂

chrisbobbe · 2021-03-22T17:48:22Z

 // only apply the device level, not within the app. See
 // https://github.com/zulip/zulip-mobile/issues/4009#issuecomment-619280681.
-export type ThemeName = 'default' | 'night';
+export type ThemeName = 'default' | 'night' | 'automatic';


Let's separate the work of

using the new UI with a new screen and a FlatList, and

adding the third, "automatic", option to the list of options

into separate commits. That way, it'll be easier to consider the details of how each thing is done. 🙂

chrisbobbe · 2021-03-22T17:50:23Z

+];
+
+class ThemeScreen extends PureComponent<Props> {
+  static contextType = ThemeContext;


nit: ThemeScreen could begin its life as a function component, instead of a class component. It's not highly urgent to convert all our components to function components that use React Hooks, but it seems like the way of the future, and I think it'd be easy to write this component as a function component.

Made it a functional component 😄

chrisbobbe · 2021-03-22T17:52:47Z

+
+  // A helper function to avoid WET(Write Everything Twice) code.
+  // Used to dispatch the correct theme which
+  // it will get from handleThemeChange function.


If I'm understanding correctly, I think this.dispatchTheme is in fact called only once, not twice. Do you expect that we'll eventually have to call it from somewhere other than handleThemeChange?

If not, I think it's simpler to just keep this code inside handleThemeChange; it means one less layer to look through when reading the code. 🙂

chrisbobbe

Thanks, @Gautam-Arora24! See some comments below.

chrisbobbe · 2021-04-01T00:32:36Z

-class ThemeProvider extends PureComponent<Props> {
-  static defaultProps = {
+function ThemeProvider(props: Props) {
+  ThemeProvider.defaultProps = {


nit: Greg has a neat explanation for a way we can make code like this more readable, without using ThemeProvider.defaultProps = { ... }: #4440 (comment)

chrisbobbe · 2021-04-01T00:35:01Z

+}
+
+export default connect(state => ({
+  theme: getSettings(state).theme,


nit: Since ThemeScreen is already a function component, we don't have to use connect from react-redux; instead, we could use useSelector, as in 641cb16, for example. Let's do that; I think it makes things simpler, and it means one less item in the component tree. 🙂

Ahh, it looks reasonable to use useSelector along with useDispatch. So now it will look something like this ->
const theme = useSelector(state => getSettings(state).theme).

chrisbobbe · 2021-04-01T00:36:08Z

+      <FlatList
+        ItemSeparatorComponent={OptionDivider}
+        data={themeData}
+        value


What are we trying to do by passing true for FlatList's value prop in this commit (the second one in this revision, "ThemeScreen: Add ThemeScreen file and wire it with AppNavigator.")?

Removed the value prop, since it's of no use.

chrisbobbe · 2021-04-01T00:54:19Z

+        data={themeData}
+        value
+        renderItem={({ item }) => (
+          <Touchable>


In this revision, I find that in the third commit—

d2517a2 SettingsScreen: Add navigation logic to navigate to ThemeScreen.

—it's impossible for a user to change the theme at all. 🙂 Let's fix it up so that no commits introduce regressions.

In particular, none of these buttons respond to touch:

(Which makes sense; I see that the Touchable component here isn't given an onPress.)

Also, I'm seeing the "System Default" button at that commit, which is earlier in the PR than I expected to see it.

Here's what I'm thinking:

The PR starts by changing from the current switch UI to this new UI with a new "Theme" screen—but, as it does so, it maintains the property that we can only switch between the two existing options; there is no third option (yet).

Then, the PR adds the third option and sets it up to work.

Steps 1 and 2 can each be separated into multiple commits, as appropriate, but I'd really like to see a separation between 1 and 2 so that we can focus conversation and review on each part. 🙂

Here is how I am following the commit sequence for this PR now ->

First commit changes the "Night Mode" label to "Theme".

Second commit introduces a new component called 'ThemeScreen' with just two theme options (Dark and Light).

Third commit adds the navigation logic and wires the 'ThemeScreen' with AppNavigator.

Fourth commit is NFC commit which converts the class-based ThemeProvider to a functional component.

Adding the logic for the third theme option called "System Default" (WIP).

chrisbobbe · 2021-04-01T00:59:02Z

+          <Touchable onPress={() => handleThemeChange(item)}>
+            <View style={styles.listItem}>
+              <View style={styles.listWrapper}>
+                <Text style={{ color }}>{item.name}</Text>


Instead of using Text with a color style, let's use Label, which, to quote from its jsdoc, "ensures consistent styling for the default and night themes".

Does this means I have to only replace the Text with Label?

chrisbobbe · 2021-04-01T01:04:32Z

+  listItem: {
+    flexDirection: 'row',
+    alignItems: 'center',
+    paddingTop: 16,


It looks like these styles were borrowed from LanguagePickerItem, but with paddingTop and paddingBottom set to 12 instead of 16. Can you talk about the choice for what goes in styles?

Ahh, I think I missed the changes I made to the styles. I am now using the same styles as mentioned in the LanguagePickerItem i.e setting the paddingTop and paddingBottom to 12 again.

chrisbobbe · 2021-04-01T01:17:06Z

+  const colorScheme = useColorScheme();
+  let themePreferred = theme;
+
+  if (themePreferred === 'automatic') {


I think themeToUse might be a bit better name than themePreferred, though I see that it's tricky to find the best names for things when we've already got theme and colorScheme floating around. 🤔

The reason is that this variable holds the answer (which we arrive at with help from theme and colorScheme) to the question, "So what theme should we actually use?". And, in particular, it doesn't represent the user's preference in the same sense that the user expresses a preference for light or dark when they press one of those items in the new Theme screen. (Because this variable might be light or dark as a result of following the OS-level setting.)

Originally discussed here: #4440 (comment) (with a bit of additional rationale) when this question first came up. It's come up a couple more times since then, as we've continued converting a lot of our components to functions with Hooks: #4555 (comment) #4699 (comment) so it's definitely time to have it written down in a proper place.

This commit changes the "Night Mode" label to "Theme".

This commit introduces a new component called 'ThemeScreen' which will be used to provide different options (currently 'Dark' and 'Light') to change the theme in the app. Fixes part of zulip#4009

Gautam-Arora24 · 2021-04-30T13:59:12Z

Thanks, @Gautam-Arora24! In particular, I appreciate your attention to the commit message! 🙂 See some comments below.

Also, there's a point mentioned at #4009 (comment) about changing the name for the 'default' theme to something else, like 'light', and Greg also mentions at #4009 (comment) that the naming for the dark mode/theme should use 'dark' instead of 'night'. Let's do those renames in a separate NFC commit or two; could go before or after the functional changes of the PR. Or it could go in a separate PR as a followup, if you'd prefer to focus on the more exciting changes in this PR (we should just be sure not to forget about it). 🙂

I will be making a separate followup PR for this part once this PR is merged 🙂

Create a new action called 'navigateToThemeScreen' which will help the 'Theme' OptionButton to navigate to 'ThemeScreen' component on clicking. Also wired it up with the 'AppNavigator'. Since, the logic for changing of theme is handled by the 'ThemeScreen' so there is no use of 'handleThemeChange' function in the 'SettingsScreen'. Similarly there is no use of Dispatch and the theme prop so all these are removed in this commit. Fixes part of zulip#4009

Convert to functional component. Make use of 'useSelector` hook instead of 'connect'. Removed Dispatch and the theme props since they are not required now.

Along with "night" and "default", made a third option called "automatic". This option will check the system settings and automatically provide the correct theme to ThemeProvider's value prop. Discussion of this in CZO [1]. [1] https://chat.zulip.org/#narrow/stream/48-mobile/topic/Automatic.20dark.2Flight.20preference Fixes part of zulip#4009

gnprice · 2022-09-23T21:00:51Z

Thanks @Gautam-Arora24 for your work on this! Closing, as I think subsequent developments have simplified what's needed here so that a fresh PR for this issue will look rather different: #4009 (comment)

Gautam-Arora24 marked this pull request as draft March 21, 2021 18:30

Gautam-Arora24 force-pushed the theme_preference branch from 10a7c53 to 75a8c12 Compare March 22, 2021 17:10

chrisbobbe requested changes Mar 22, 2021

View reviewed changes

Gautam-Arora24 force-pushed the theme_preference branch 4 times, most recently from 526ad0c to f6ed310 Compare March 24, 2021 19:27

chrisbobbe requested changes Apr 1, 2021

View reviewed changes

Gautam-Arora24 force-pushed the theme_preference branch from f6ed310 to 4ac5abb Compare April 29, 2021 11:51

Gautam-Arora24 marked this pull request as ready for review April 29, 2021 11:55

Gautam-Arora24 added 2 commits April 30, 2021 01:16

SettingsScreen: Change the "Night Mode" label to "Theme".

edb227e

This commit changes the "Night Mode" label to "Theme".

ThemeScreen: Create a basic ThemeScreen component.

f90eb10

This commit introduces a new component called 'ThemeScreen' which will be used to provide different options (currently 'Dark' and 'Light') to change the theme in the app. Fixes part of zulip#4009

Gautam-Arora24 force-pushed the theme_preference branch from 4ac5abb to fbfcd86 Compare April 29, 2021 20:00

Gautam-Arora24 changed the title ~~feat: Automatically follow user's OS-level dark/light preference.~~ Automatically follow user's OS-level dark/light preference. Apr 30, 2021

Gautam-Arora24 added 3 commits May 1, 2021 19:31

ThemeProvider [nfc]: Convert to functional component.

f2c7a79

Convert to functional component. Make use of 'useSelector` hook instead of 'connect'. Removed Dispatch and the theme props since they are not required now.

Gautam-Arora24 force-pushed the theme_preference branch from fbfcd86 to b82d99a Compare May 1, 2021 14:02

gnprice closed this Sep 23, 2022

Uh oh!

Conversation

Gautam-Arora24 commented Mar 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Previous Functionality

Changes Done

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gautam-Arora24 Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gautam-Arora24 Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gautam-Arora24 commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Sep 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Gautam-Arora24 commented Mar 21, 2021 •

edited

Loading

Gautam-Arora24 Apr 29, 2021 •

edited

Loading

Gautam-Arora24 Apr 29, 2021 •

edited

Loading

Gautam-Arora24 commented Apr 30, 2021 •

edited

Loading