Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: Refactor & adjustments for better functionality #14

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jordanvidrine
Copy link
Contributor

When users who had their system set to dark mode used the toggle light scheme, the logo would remain as a dark-mode logo. This has now been fixed.

I made the toggle appearing in the header the default in addition to appearing in the hamburger menu. I also changed the setting wording to "remove" the toggle from the header should an admin decide to do so.

@CvX CvX changed the title FIX: Refactor & adjustments for better fucntionality FIX: Refactor & adjustments for better functionality Nov 14, 2022
settings.yml Outdated Show resolved Hide resolved
@@ -70,6 +70,7 @@ Have you selected two different themes for your dark/light schemes in user prefe
lightTheme.media = "none";

Session.currentProp("defaultColorSchemeIsDark", true);
Session.currentProp("darkModeAvailable", true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's that for? Is it used (read) in this theme component? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I found that if your system is set to dark mode, and you click toggle light color scheme the logo would not change. Adding these Session.currentProp methods fixed this. Here is where it is used in core.

https://github.com/discourse/discourse/blob/38bb00edce0764d3e0c4700115622af446c84db5/app/assets/javascripts/discourse/app/widgets/home-logo.js#L37

The only draw back is now, when changing your system from either dark to light, and this component is set to auto, the page needs a refresh to load the correct logo. I feel the middle ground I've come to is fine for this component.

I also contemplated turning off the auto select.

@jordanvidrine jordanvidrine requested a review from CvX November 14, 2022 23:46
@pmusaraj
Copy link
Contributor

I pulled this locally @jordanvidrine and the original issue here seems to be due to these lines:

https://github.com/discourse/discourse-color-scheme-toggle/blob/main/javascripts/discourse/initializers/dark-light-toggle-hamburger.js#L94-L98

      if (window?.matchMedia("(prefers-color-scheme: dark)").matches) {
        Session.currentProp("defaultColorSchemeIsDark", true);
      } else {
        Session.currentProp("defaultColorSchemeIsDark", false);
      }

When this component sets defaultColorSchemeIsDark the core home-logo component assumes that the only available logo is the dark mode one. Generally speaking, I don't think this component should set that session property, because in core it is inherited from the backend and it is there just to indicate whether the default color scheme is dark (core needs to know that so it doesn't load a second dark mode stylesheet).

Instead, I think the component can directly manipulate the media property of the two color definition stylesheets:

image

When cookie is set to light, set light scheme stylesheet to media="all", dark scheme stylesheet to media="none" (not sure if syntax is correct for the latter).
When cookie is set to dark, do the opposite. And when auto is selected, reset to core defaults (like in screenshot above).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants