gear_menu: Add theme switcher to the gear menu popover. #30334

sayamsamal · 2024-06-06T10:39:09Z

This PR improves the theme experience for spectators by:

Adding a Theme Switcher: Replaces separate light/dark theme options with the new 3-way theme switcher in the gear menu popover, standardizing theme selection across the app.
Updating the Realm Logo: Ensures the realm logo dynamically updates on theme change without requiring a page reload.
Supporting Automatic Color Scheme: Implements automatic color scheme support by storing the user's preference in local storage, mirroring the light/dark theme functionality.

Screenshots and screen captures:

Desc	Before	After
Light Mode
Dark Mode
Dynamic Realm Logo on theme change

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

sentry-io · 2024-06-06T10:39:42Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: web/src/ui_init.js

Function	Unhandled Issue
`initialize_everything`	Error: Unknown user_id in get_by_user_id: 31266 g... `Event Count:` 9 `Affected Users:` 3
`initialize_everything`	TypeError: p()("input.stream-list-filter").expectOne is not a function. (In 'p()("input.stream-list-filter")... ... `Event Count:` 2 `Affected Users:` 3
`initialize_everything`	ReferenceError: Can't find variable: ResizeObserver compose_setup(src/compose_set... `Event Count:` 1 `Affected Users:` 2

_{Did you find this useful? React with a 👍 or 👎}

sayamsamal · 2024-06-06T11:31:26Z

@amanagr Could you please review this PR? Thanks!

amanagr · 2024-06-06T12:37:57Z

web/src/dark_theme.ts

@@ -22,9 +24,17 @@ export function disable(): void {
        const ls = localstorage();
        ls.set("spectator-theme-preference", "light");
        user_settings.color_scheme = settings_config.color_scheme_values.day.code;
+        realm_logo.render();
    }
 }

 export function default_preference_checker(): void {


Should we rename this function and library? dark_theme.default_preference_checker is a weird thing to call.

I would prefer it to be called theme.use_system_default instead.

Let's open a czo thread to weight in opinion from others.

Opened a CZO thread discussing this idea, thanks!

amanagr · 2024-06-06T12:41:45Z

web/src/dark_theme.ts

+        const ls = localstorage();
+        ls.set("spectator-theme-preference", "automatic");
+        user_settings.color_scheme = settings_config.color_scheme_values.automatic.code;
+        realm_logo.render();


Let's extract this into a function since it seems like it would make this easier to look at and of course has the benefit of DRY.

Extracted this and other such methods for setting spectator theme into dark_theme.set_theme_for_spectator().

amanagr · 2024-06-06T12:50:14Z

web/src/gear_menu.js

+                const theme_code = Number.parseInt($(e.currentTarget).attr("data-theme-code"), 10);
                requestAnimationFrame(() => {
-                    dark_theme.disable();
+                    if (theme_code === settings_config.color_scheme_values.night.code) {
+                        dark_theme.enable();
+                    } else if (theme_code === settings_config.color_scheme_values.day.code) {
+                        dark_theme.disable();
+                    } else {
+                        dark_theme.default_preference_checker();
+                    }
                    message_lists.update_recipient_bar_background_color();


Let's move this call inside dark_theme and calling it in the server events dispatch path too.

Moved the CSS class logic into dark_theme.set_theme() and added dark_theme.set_theme_and_update(), which calls set_theme() and also calls the realm logo and recipient bar update methods. The idea is that all theme switchers and commands call dark_theme.set_theme_and_update(), which will set the theme and also update the necessary elements. While dark_theme.set_theme() is used in ui_init.js to only set the theme via the CSS classes, since the theme is set before the message list and the realm logo are initialised.

amanagr · 2024-06-06T12:51:04Z

web/src/dark_theme.ts

@@ -12,6 +13,7 @@ export function enable(): void {
        const ls = localstorage();
        ls.set("spectator-theme-preference", "dark");
        user_settings.color_scheme = settings_config.color_scheme_values.night.code;
+        realm_logo.render();


I was wondering why we don't need realm_logo.render(); call for logged-in users and it seems like we do, so we move the call inside these enable/ disable functions as well.

Makes sense, thanks! Moved realm_logo.render() along with message_lists.update_recipient_bar_background_color() into dark_theme module.

amanagr · 2024-06-06T12:51:45Z

@sayamsamal thanks for working on this! Looks like this library and the code surrounding it needs some long due cleanup. Let's make it happen!

Similar to the light/dark theme support for the spectators, this adds the automatic color scheme support by storing the user's preference in the local storage.

Before this, the realm logo was not being updated instantly when the theme was changed through the gear menu, and instead required a page reload to take effect.

Standardize theme selection across the web app by replacing separate light/dark theme menu options being used in the spectator view with the new 3-way theme switcher. Fixes zulip#30318.

This commit centralizes the logic for setting a user's theme preference, both for regular users and spectators, into the `dark_theme.ts` module. This simplifies theme handling throughout the codebase and ensures that the theme is set consistently across all modules. Instead of relying on various call sites to update the recipient bar's background color and switch between the light/dark realm logo after a theme change, this commit modifies the `set_theme_and_update` function to include these calls after every theme change. Before this commit, some modules used to update the realm logo after a theme change, while others did not. This led to inconsistencies in the UI depending on which method was used to change the theme.

The dark_theme module now contains logic for light, dark, and automatic theme switching. Thus, we rename it a more generic name, `theme.ts`.

As a follow-up to the previous commit renaming the `dark_theme.ts` module to `theme.ts`, this commit renames the following functions: - `enable` -> `set_dark_theme` - `disable` -> `set_light_theme` - `default_preference_checker` -> `set_automatic_theme`

This commit standardizes the naming of the day and night themes to light and dark, respectively. This makes the codebase more consistent with the naming used in the settings and the user interface.

zulipbot · 2024-06-21T21:50:58Z

Heads up @sayamsamal, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

zulipbot added size: L area: public access Issues related to logged-out access in Zulip priority: high labels Jun 6, 2024

sayamsamal requested a review from amanagr June 6, 2024 10:40

sayamsamal added the maintainer review PR is ready for review by Zulip maintainers. label Jun 6, 2024

amanagr reviewed Jun 6, 2024

View reviewed changes

sayamsamal force-pushed the spectator-view-theme-switcher branch from d54c01c to 51653d4 Compare June 10, 2024 19:38

zulipbot added size: XL and removed size: L labels Jun 10, 2024

sayamsamal force-pushed the spectator-view-theme-switcher branch from 51653d4 to 82c3bfb Compare June 10, 2024 21:37

zulipbot added the has conflicts label Jun 11, 2024

sayamsamal and others added 7 commits June 20, 2024 01:13

dark_theme: Add automatic color scheme support for spectators.

233593a

Similar to the light/dark theme support for the spectators, this adds the automatic color scheme support by storing the user's preference in the local storage.

realm_logo: Render light/dark realm logo on theme change for spectators.

7886c9d

Before this, the realm logo was not being updated instantly when the theme was changed through the gear menu, and instead required a page reload to take effect.

gear_menu: Add theme switcher to the gear menu popover.

523a75d

Standardize theme selection across the web app by replacing separate light/dark theme menu options being used in the spectator view with the new 3-way theme switcher. Fixes zulip#30318.

theme: Rename dark_theme.ts -> theme.ts.

783aa02

The dark_theme module now contains logic for light, dark, and automatic theme switching. Thus, we rename it a more generic name, `theme.ts`.

theme: Rename day->light and night->dark in the frontend code.

b7270ae

This commit standardizes the naming of the day and night themes to light and dark, respectively. This makes the codebase more consistent with the naming used in the settings and the user interface.

sayamsamal force-pushed the spectator-view-theme-switcher branch from 82c3bfb to b7270ae Compare June 20, 2024 17:44

zulipbot removed the has conflicts label Jun 20, 2024

zulipbot added the has conflicts label Jun 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gear_menu: Add theme switcher to the gear menu popover. #30334

gear_menu: Add theme switcher to the gear menu popover. #30334

sayamsamal commented Jun 6, 2024 •

edited

Loading

sentry-io bot commented Jun 6, 2024

sayamsamal commented Jun 6, 2024

amanagr Jun 6, 2024

amanagr Jun 6, 2024

sayamsamal Jun 10, 2024

amanagr Jun 6, 2024

sayamsamal Jun 10, 2024

amanagr Jun 6, 2024

sayamsamal Jun 10, 2024 •

edited

Loading

amanagr Jun 6, 2024

sayamsamal Jun 10, 2024

amanagr commented Jun 6, 2024

zulipbot commented Jun 21, 2024

gear_menu: Add theme switcher to the gear menu popover. #30334

Are you sure you want to change the base?

gear_menu: Add theme switcher to the gear menu popover. #30334

Conversation

sayamsamal commented Jun 6, 2024 • edited Loading

sentry-io bot commented Jun 6, 2024

🔍 Existing Issues For Review

sayamsamal commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sayamsamal Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanagr commented Jun 6, 2024

zulipbot commented Jun 21, 2024

sayamsamal commented Jun 6, 2024 •

edited

Loading

sayamsamal Jun 10, 2024 •

edited

Loading