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.

Sign up for GitHub

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

Make system setting the default theme preference #1581

Merged
merged 4 commits into from
Oct 16, 2019
Merged

Make system setting the default theme preference #1581

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8d0947d
Make system setting the default theme preference
belcherj Sep 23, 2019
1e9bed9
Update Release Notes
belcherj Sep 23, 2019
e862ed4
Make system the first item in the list
belcherj Oct 7, 2019
644ed53
Update system theme logic
belcherj Oct 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Future release

### Enhancements

- Add ability to select system as a theme option and make it the default

### Fixes

- Rework WordPress.com signin to prevent infinite looping and login failures [#1627](https://github.com/Automattic/simplenote-electron/pull/1627)
Expand Down
4 changes: 4 additions & 0 deletions desktop/menus/view-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ const buildViewMenu = settings => {
{
label: 'T&heme',
submenu: [
{
label: '&System',
Copy link
Member

Choose a reason for hiding this comment

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

Is this the label we use on the other apps? Is System enough to explain System setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I struggled with what to call this option for the setting. Is it Default? Is it OS Setting? System seemed to be the best answer.

id: 'system',
},
{
label: '&Light',
id: 'light',
Expand Down
3 changes: 2 additions & 1 deletion lib/app.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { Component } from 'react';
import PropTypes from 'prop-types';
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
import getTheme from './utils/get-theme';
import 'focus-visible/dist/focus-visible.js';
import appState from './flux/app-state';
import { loadTags } from './state/domain/tags';
Expand Down Expand Up @@ -404,7 +405,7 @@ export const App = connect(
} = this.props;
const isMacApp = isElectronMac();

const themeClass = `theme-${settings.theme}`;
const themeClass = `theme-${getTheme(settings.theme, isElectron())}`;

const appClasses = classNames('app', themeClass, {
'is-line-length-full': settings.lineLength === 'full',
Expand Down
1 change: 1 addition & 0 deletions lib/dialogs/settings/panels/display.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ const DisplayPanel = props => {
onChange={actions.activateTheme}
renderer={RadioGroup}
>
<Item title="System" slug="system" />
<Item title="Light" slug="light" />
<Item title="Dark" slug="dark" />
</SettingsGroup>
Expand Down
2 changes: 1 addition & 1 deletion lib/state/settings/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export const initialState = {
sortTagsAlpha: false,
sortType: 'modificationDate',
spellCheckEnabled: true,
theme: 'light',
theme: 'system',
wpToken: false,
};

Expand Down
12 changes: 12 additions & 0 deletions lib/utils/get-theme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export default (themeSetting, isElectron) => {
if (themeSetting === 'system') {
if (isElectron) {
const { remote } = __non_webpack_require__('electron'); // eslint-disable-line no-undef
return remote.systemPreferences.isDarkMode ? 'dark' : 'light';
}
return window.matchMedia('(prefers-color-scheme: dark)').matches
? 'dark'
: 'light';
}
return themeSetting;
};
Copy link
Member

Choose a reason for hiding this comment

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

isElectron is something that won't change/can't change during runtime. because of that we won't need to pass it in to the function call on every invocation.

maybe it's easier to pass it in from app.jsx where we make the check, though it seems awkward.

it's probably also awkward to think about defining this function in app.jsx, but it would remove the arg-passing and place this directly next to where it's called (or in the same file at least vs. through a redirect)

okay so this is bad but here's an idea. it hooks into the point where we are supposedly initializing Electron specifics and replaces the getTheme function; by keeping it contained in app.jsx it removes some abstraction that maybe doesn't need to be there. it's less testable, but we're already depending on remote.systemPreferences and window.matchMedia which are themselves difficult to test

class App extends Component {

  initializeElectron = () => {
    const { remote } = __non_webpack_require('electron');

    // replace theme selection to get system setting
    this.getTheme = () => {
      const { settings: { theme } } = this.props;

      return 'system' === theme
        ? ( remote.systemPreferences.isDarkMode ? 'dark' : 'light' ) 
        : theme;
    }

    this.setState( … );
  }

  …

  getTheme = () => {
    const { settings: { theme } } = this.props;

    return 'system' === theme
      ? ( window.matchMedia( '(prefers-color-scheme: dark)' ).matches ? 'dark' : 'light' )
      : theme;
  }

  …
}

broken apart…

getSystemColorMode = () => window.matchMedia('(prefers-color-scheme: dark)').match ? 'dark' : 'light';

getTheme = () => {
  const { settings: { theme } } = this.props;

  return 'system' === theme
    ? this.getSystemColorMode()
    : theme;
}

initializeElectron = () => {
  …

  this.getSystemColorMode = () => remote.systemPreferences.isDarkMode ? 'dark' : 'light';

  …
}