Renamed "Night mode" to "Dark theme" throughout the app

[aritroCoder]

Fixes #5169
The app is stable and working fine after the changes

[chrisbobbe]

Here's a screenshot of your branch from the GitHub UI:

Could you please clean it up? I see some merge commits in there; the Zulip project doesn't use those. And for the remaining commits, please make sure they're clear and coherent according to our style, with properly formatted commit messages and everything.

Thanks!

[aritroCoder]

Here's a screenshot of your branch from the GitHub UI:

Could you please clean it up? I see some merge commits in there; the Zulip project doesn't use those. And for the remaining commits, please make sure they're clear and coherent according to our style, with properly formatted commit messages and everything.

Thanks!

You can see the files changed section, I havent made any changes other than the renaming. These commits were there mainly because I solved few merge conflicts, and made few mistakes like accidentaly changing the gradle version which I restored. I think you can merge all of this in a single commit in the main branch if that is fine

[chrisbobbe]

Please do read through our style guide. If you'd like to organize your work into one commit (which seems reasonable in this case), please write that commit and remove the others. 🙂 If you need any help, feel free to ask in the #git help stream in the Zulip development community.

To avoid another round trip, here are some small things I noticed from a quick look at that "Files changed" tab:

• In static/translations/, please only change messages_en.json. The other messages_*.json files will be updated programmatically when a maintainer runs tools/tx-sync. For more about our translation process, see our docs/howto/translations.md.
• Let's leave docs/changelog.md unchanged. In the versions described by those changelog entries, the feature really was called "night mode".

And one substantive thing:

• As I pointed out at Rename "Night mode" to "Dark theme" #5169 (comment), we'll need a migration. Does it make sense how migrations work? You mentioned that you added the migration, so it's possible that you just haven't pushed your work for that yet.

[aritroCoder]

Please do read through our style guide. If you'd like to organize your work into one commit (which seems reasonable in this case), please write that commit and remove the others. slightly_smiling_face If you need any help, feel free to ask in the #git help stream in the Zulip development community.

To avoid another round trip, here are some small things I noticed from a quick look at that "Files changed" tab:

• In static/translations/, please only change messages_en.json. The other messages_*.json files will be updated programmatically when a maintainer runs tools/tx-sync. For more about our translation process, see our docs/howto/translations.md.
• Let's leave docs/changelog.md unchanged. In the versions described by those changelog entries, the feature really was called "night mode".

And one substantive thing:

• As I pointed out at Rename "Night mode" to "Dark theme" #5169 (comment), we'll need a migration. Does it make sense how migrations work? You mentioned that you added the migration, so it's possible that you just haven't pushed your work for that yet.

Is this what I have to do for the migration? In src/storage/migrations.js

  // Add presenceEnabled to state.realm.
  '56': dropCache,

  // Rename form 'night' to 'dark' in state.settings.theme.  
  '57': state => {
    const { theme, ...settingsRest } = state.settings;
    return {
      ...state,
      settings: {
        ...settingsRest,
        theme: 'default' | 'dark',
      },
    };
  },

  // TIP: When adding a migration, consider just using `dropCache`.
  //   (See its jsdoc for guidance on when that's the right answer.)
};

[chrisbobbe]

Hmm, what do you mean with theme: 'default' | 'dark',?

This is the migration I would add, and some test cases for it:

diff --git src/storage/__tests__/migrations-test.js src/storage/__tests__/migrations-test.js
index 23f4cd269..4c3bbacee 100644
--- src/storage/__tests__/migrations-test.js
+++ src/storage/__tests__/migrations-test.js
@@ -104,7 +104,7 @@ describe('migrations', () => {
   // What `base` becomes after all migrations.
   const endBase = {
     ...base52,
-    migrations: { version: 56 },
+    migrations: { version: 57 },
   };
 
   for (const [desc, before, after] of [
@@ -278,6 +278,20 @@ describe('migrations', () => {
       },
       { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'never' } },
     ],
+    [
+      "check 57 with 'night'",
+      { ...base52, migrations: { version: 56 }, settings: { ...base52.settings, theme: 'night' } },
+      { ...endBase, settings: { ...endBase.settings, theme: 'dark' } },
+    ],
+    [
+      "check 57 with 'default'",
+      {
+        ...base52,
+        migrations: { version: 56 },
+        settings: { ...base52.settings, theme: 'default' },
+      },
+      { ...endBase, settings: { ...endBase.settings, theme: 'default' } },
+    ],
   ]) {
     /* eslint-disable no-loop-func */
     test(desc, async () => {
diff --git src/storage/migrations.js src/storage/migrations.js
index b169c9617..176fbe3ca 100644
--- src/storage/migrations.js
+++ src/storage/migrations.js
@@ -472,6 +472,12 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
   // Add presenceEnabled to state.realm.
   '56': dropCache,
 
+  // Rename 'night' to 'dark' in state.settings.theme
+  '57': state => ({
+    ...state,
+    settings: { ...state.settings, theme: state.settings.theme === 'night' ? 'dark' : 'default' },
+  }),
+
   // TIP: When adding a migration, consider just using `dropCache`.
   //   (See its jsdoc for guidance on when that's the right answer.)
 };

[aritroCoder]

Hmm, what do you mean with theme: 'default' | 'dark',?

This is the migration I would add, and some test cases for it:

diff --git src/storage/__tests__/migrations-test.js src/storage/__tests__/migrations-test.js
index 23f4cd269..4c3bbacee 100644
--- src/storage/__tests__/migrations-test.js
+++ src/storage/__tests__/migrations-test.js
@@ -104,7 +104,7 @@ describe('migrations', () => {
   // What `base` becomes after all migrations.
   const endBase = {
     ...base52,
-    migrations: { version: 56 },
+    migrations: { version: 57 },
   };
 
   for (const [desc, before, after] of [
@@ -278,6 +278,20 @@ describe('migrations', () => {
       },
       { ...endBase, settings: { ...endBase.settings, markMessagesReadOnScroll: 'never' } },
     ],
+    [
+      "check 57 with 'night'",
+      { ...base52, migrations: { version: 56 }, settings: { ...base52.settings, theme: 'night' } },
+      { ...endBase, settings: { ...endBase.settings, theme: 'dark' } },
+    ],
+    [
+      "check 57 with 'default'",
+      {
+        ...base52,
+        migrations: { version: 56 },
+        settings: { ...base52.settings, theme: 'default' },
+      },
+      { ...endBase, settings: { ...endBase.settings, theme: 'default' } },
+    ],
   ]) {
     /* eslint-disable no-loop-func */
     test(desc, async () => {
diff --git src/storage/migrations.js src/storage/migrations.js
index b169c9617..176fbe3ca 100644
--- src/storage/migrations.js
+++ src/storage/migrations.js
@@ -472,6 +472,12 @@ const migrationsInner: {| [string]: (LessPartialState) => LessPartialState |} =
   // Add presenceEnabled to state.realm.
   '56': dropCache,
 
+  // Rename 'night' to 'dark' in state.settings.theme
+  '57': state => ({
+    ...state,
+    settings: { ...state.settings, theme: state.settings.theme === 'night' ? 'dark' : 'default' },
+  }),
+
   // TIP: When adding a migration, consider just using `dropCache`.
   //   (See its jsdoc for guidance on when that's the right answer.)
 };

Thanks for this, I think I can do it now. So to clean the commit history of my branch, I will create a new pull request with the required commits, right?

[chrisbobbe]

So to clean the commit history of my branch, I will create a new pull request with the required commits, right?

There's an easier way: you can fix your commits locally, then force-push to the same PR branch, so you don't have to make a new PR. 🙂 (In fact, that's preferred, so we don't have to keep track of work across separate PRs.)

If you have more questions or run into trouble, please post in the #git help stream in the Zulip development community, where it's easier to have more detailed debugging conversations.

[aritroCoder]

@chrisbobbe I hope it's fine to merge now

[chrisbobbe]

Thanks, much better! 🙂

See a few small comments below. Also, your commit message isn't yet formatted according to our style. Right now it's missing a prefix ('settings:' is fine) and a Fixes: line, and it should say "Rename" instead of "Renamed".

If you don't have a graphical Git client set up, I highly recommend it. One thing it's useful for is to see examples of merged commits that follow our style. Greg's "secret" to using git log -p is also very useful for that. These points are covered in the zulip-mobile project's Git guide.

[chrisbobbe]

Bump on this part of #5595 (comment):

Let's leave docs/changelog.md unchanged. In the versions described by those changelog entries, the feature really was called "night mode".

[chrisbobbe]

It looks like you accidentally left this in; please amend your commit to remove the changes in static/translations/messages_tl.json.

[chrisbobbe]

Thanks! Small comments below, and the commit message still doesn't follow our style; this is all covered in the docs I've linked to:

Fixes: Rename 'night mode' to 'dark theme' (#2)

Changed night mode to dark theme in the app to avoid difference between the web and app interface of zulip(web-app parity)
App works fine under the testing environments

• In the summary line, the prefix should refer to a subsystem; I've suggested 'settings:' for that.
• The body should be wrapped to ideally 68 characters but no more than 70.
• There should be a line at the end with Fixes: #5169

To see commit-message examples, please use a graphical Git client to view the project's history, or Greg's "secret" to using git log -p; I've linked to a doc with instructions for that.

[chrisbobbe]

nit: keep newline at end of file

[chrisbobbe]

nit: keep newline at end of file

[gnprice]

Thanks @aritroCoder!

Searching for "night" in the codebase (git grep -i night) finds one more thing to update: the file cssNight.js, along with a couple of places it's referred to. Let's make that change as a separate commit from the rest.

Other than that, just small comments:

• @chrisbobbe's comment above about the commit-message format is still open.
• One nit below.

[gnprice]

nit: keep existing capitalization

[aritroCoder]

@chrisbobbe @gnprice I made the changes some days ago, did you review it?

[chrisbobbe]

Hi @aritroCoder, thank you!

These are the three commits in this revision:

fff58b3 settings: Rename 'night mode' to 'dark theme'
97b3fa3 settings: rename cssNight.js to cssDark.js
6530a23 settings: rename cssNight.js to cssDark.js

A nit for the second commit: please capitalize the first letter after the colon, so settings: Rename cssNight.js to cssDark.js.

Also, please fix the one remaining place in src/emoji/codePointMap.js where Greg's comment about capitalization applies: #5595 (comment)

It looks like the third commit is a merge commit:

$ git log --stat -p --no-decorate
commit 6530a23c7262ad0e5dc520bdac6aa8a16b26f690
Merge: fff58b37e 97b3fa35c
Author: Aritra Bhaduri <92646038+aritroCoder@users.noreply.github.com>
Date:   Sun Dec 18 10:43:19 2022 +0530

    settings: rename cssNight.js to cssDark.js
    
    Rename cssNight.js to cssDark.js and update required imports

So please remove that commit.

[chrisbobbe]

Please let us know, by commenting here, when this is ready for another review.

[aritroCoder]

Please let us know, by commenting here, when this is ready for another review.

Yes you can start reviewing, can you say why the check is failing? I did not make any major changes

[chrisbobbe]

In this revision, the changes to src/settings/__tests__/settingsReducer-test.js look like they might have come from a wrong rebase conflict resolution.

My last review suggested fixing your branch to have just two new commits: #5595 (comment)

Could you please do that, or else explain why this revision's six commits are the ones you want to land in main: