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

aritroCoder · 2022-12-09T04:40:59Z

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

chrisbobbe · 2022-12-13T04:19:24Z

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 · 2022-12-13T04:59:38Z

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 · 2022-12-13T05:25:29Z

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 · 2022-12-13T07:13:09Z

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 · 2022-12-13T08:56:36Z

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 · 2022-12-13T13:48:15Z

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 · 2022-12-13T19:42:27Z

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 · 2022-12-14T07:42:01Z

@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 · 2022-12-14T07:50:12Z

docs/changelog.md

@@ -1900,7 +1900,7 @@ Bugfixes and other improvements for your Zulip experience.
 ### Highlights for users (since 26.1.124 / 26.2.125)

 * Highlight colors for code blocks now match the webapp and
-  offer more contrast, especially in night mode.
+  offer more contrast, especially in Dark theme.


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 · 2022-12-14T07:51:28Z

static/translations/messages_tl.json

+<<<<<<< HEAD
+  "Settings": "Settings",
+  "Night mode": "Night mode",
+=======
  "Settings": "Mga Itinakda",
  "Night mode": "Night mode",
+>>>>>>> c8af6d69ecbed660643facc1e84b416e3dbff879


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 · 2022-12-14T20:08:19Z

docs/changelog.md

-TODO?: backfill some of this information from notes in other places.
+TODO?: backfill some of this information from notes in other places.


nit: keep newline at end of file

chrisbobbe · 2022-12-14T20:10:05Z

static/translations/messages_tl.json

-}
+}


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 · 2022-12-16T01:47:11Z

src/emoji/codePointMap.js

-  // than we want, plus when not in night mode it's actually invisible.
+  // than we want, plus when not in Dark theme it's actually invisible.


nit: keep existing capitalization

Fixes: zulip#5169 Changed night mode to dark theme. Added tests, migrations, and updated comments.

Rename cssNight.js to cssDark.js and update required imports

aritroCoder · 2022-12-22T13:46:26Z

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

chrisbobbe · 2022-12-22T17:24:59Z

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 <[email protected]>
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.

Fixes: zulip#5169 Changed night mode to dark theme. Added tests, migrations, and updated comments.

Rename cssNight.js to cssDark.js and update required imports

chrisbobbe · 2022-12-27T15:55:25Z

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

aritroCoder · 2022-12-27T15:56:53Z

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 · 2022-12-27T16:32:54Z

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:

aritroCoder force-pushed the main branch from fd6abf2 to cd6f88e Compare December 14, 2022 07:35

aritroCoder closed this Dec 14, 2022

aritroCoder force-pushed the main branch from cd6f88e to b6218db Compare December 14, 2022 07:37

aritroCoder reopened this Dec 14, 2022

chrisbobbe requested changes Dec 14, 2022

View reviewed changes

aritroCoder force-pushed the main branch from 725a5ca to 7eb5a6d Compare December 14, 2022 09:07

aritroCoder closed this Dec 14, 2022

aritroCoder force-pushed the main branch from 7eb5a6d to b6218db Compare December 14, 2022 09:09

aritroCoder reopened this Dec 14, 2022

chrisbobbe requested changes Dec 14, 2022

View reviewed changes

aritroCoder force-pushed the main branch from 62aa251 to a0584ba Compare December 15, 2022 12:50

aritroCoder closed this Dec 15, 2022

aritroCoder force-pushed the main branch from a0584ba to 7cbe5cf Compare December 15, 2022 12:52

aritroCoder reopened this Dec 15, 2022

gnprice reviewed Dec 16, 2022

View reviewed changes

aritroCoder closed this Dec 18, 2022

aritroCoder force-pushed the main branch from 7cba30d to fc2dab7 Compare December 18, 2022 05:01

settings: Rename 'night mode' to 'dark theme'

fff58b3

Fixes: zulip#5169 Changed night mode to dark theme. Added tests, migrations, and updated comments.

aritroCoder reopened this Dec 18, 2022

aritroCoder and others added 2 commits December 18, 2022 10:41

settings: rename cssNight.js to cssDark.js

97b3fa3

settings: rename cssNight.js to cssDark.js

6530a23

Rename cssNight.js to cssDark.js and update required imports

aritroCoder closed this Dec 23, 2022

aritroCoder force-pushed the main branch from 6530a23 to e8ad88c Compare December 23, 2022 12:11

aritroCoder and others added 3 commits December 23, 2022 17:45

settings: Rename 'night mode' to 'dark theme'

a464938

Fixes: zulip#5169 Changed night mode to dark theme. Added tests, migrations, and updated comments.

nit: Fix capitalization of 'Dark theme' in comments

a2917f4

settings: Rename cssNight.js to cssDark.js

13f940a

Rename cssNight.js to cssDark.js and update required imports

aritroCoder reopened this Dec 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aritroCoder commented Dec 9, 2022 •

edited

Loading

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 13, 2022 •

edited

Loading

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 13, 2022 •

edited

Loading

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 13, 2022

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 14, 2022 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Dec 14, 2022

chrisbobbe Dec 14, 2022 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Dec 14, 2022

chrisbobbe Dec 14, 2022

gnprice left a comment

gnprice Dec 16, 2022

aritroCoder commented Dec 22, 2022

chrisbobbe commented Dec 22, 2022

chrisbobbe commented Dec 27, 2022

aritroCoder commented Dec 27, 2022 •

edited

Loading

chrisbobbe commented Dec 27, 2022 •

edited

Loading

		TODO?: backfill some of this information from notes in other places.
		TODO?: backfill some of this information from notes in other places.

		// than we want, plus when not in night mode it's actually invisible.
		// than we want, plus when not in Dark theme it's actually invisible.

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

Are you sure you want to change the base?

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

Conversation

aritroCoder commented Dec 9, 2022 • edited Loading

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 13, 2022 • edited Loading

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 13, 2022 • edited Loading

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 13, 2022

chrisbobbe commented Dec 13, 2022

aritroCoder commented Dec 14, 2022 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Dec 14, 2022

Choose a reason for hiding this comment

chrisbobbe Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Dec 14, 2022

Choose a reason for hiding this comment

chrisbobbe Dec 14, 2022

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice Dec 16, 2022

Choose a reason for hiding this comment

aritroCoder commented Dec 22, 2022

chrisbobbe commented Dec 22, 2022

chrisbobbe commented Dec 27, 2022

aritroCoder commented Dec 27, 2022 • edited Loading

chrisbobbe commented Dec 27, 2022 • edited Loading

aritroCoder commented Dec 9, 2022 •

edited

Loading

aritroCoder commented Dec 13, 2022 •

edited

Loading

aritroCoder commented Dec 13, 2022 •

edited

Loading

aritroCoder commented Dec 14, 2022 •

edited

Loading

chrisbobbe Dec 14, 2022 •

edited

Loading

aritroCoder commented Dec 27, 2022 •

edited

Loading

chrisbobbe commented Dec 27, 2022 •

edited

Loading