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

Enable system theme for Electron builds #2135

Closed
wants to merge 1 commit into from

Conversation

codebykat
Copy link
Member

Fix

When we added System theme (follow OS light/dark setting), the version of Electron didn't support it. As of Electron 7.0.0, it seems that it does:

https://www.electronjs.org/docs/tutorial/mojave-dark-mode-guide

This PR enables the System theme setting in both the menu bar and Settings dialog.

Will fix #2134

Test

  1. Set Electron to "System" theme and verify that it changes to whatever the OS is set to
  2. Change the OS setting and verify that Electron also changes to follow
  3. Use the light/dark themes and make sure they don't change
  4. We need to test this on Windows and Linux! I am not sure of the cross-platform support or if we need to be special-casing it.

Review

It looks to me like we're not following the docs here and accessing this property via Electron's nativeTheme API:

https://www.electronjs.org/docs/api/native-theme

However, what we're doing appears to work fine, at least on OSX. Possibly @belcherj knows more.

Release

Not updated: Add System theme for Electron builds

@codebykat codebykat requested a review from a team June 4, 2020 06:18
@codebykat codebykat self-assigned this Jun 4, 2020
@belcherj
Copy link
Contributor

belcherj commented Jun 4, 2020

This does not work in Windows. I think we will need to do some more work on Windows and Linux.

Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

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

Could you take a look at Windows and Linux?

@stpoa
Copy link

stpoa commented Jul 16, 2020

I need this one for mac :) If there is a problem for other platforms, can you split this pr to just support macos?

@codebykat
Copy link
Member Author

Superseded by #2882

@codebykat codebykat closed this Apr 30, 2021
sandymcfadden added a commit that referenced this pull request May 1, 2021
Picked up from PR #2135

Currently in our Electron builds we don't offer a system theme that responds to the OSs color preference but is available in the web app.

This enables it for both Windows and Mac builds. There appears to be a bug in Electron or Chromium on Linux that the app doesn't respond properly to changing the color preferences so it still won't work there. Possibly this will get resolved in Electron 13 due to come out May 25, 2021.

It also updates so that the system menu for Windows updates to be the choice of theme in Simplenote itself. So if you specifically choose Light the menu will be light, same for dark, overriding the system color preference.

This semi addresses issues #2134 and #1799 as they will both be resolved for Mac and Windows, but still not working on the Linux builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "System" theme (like PWA version)
3 participants