MM-23853 Support dark mode and light mode syncing with OS theme setting #8804

komik966 · 2021-09-08T11:08:27Z

Summary

This PR adds support for dark mode and light mode syncing with OS theme setting

Ticket Link

mattermost/mattermost#15975

Related Pull Requests

Has server changes MM-23853 Support dark mode and light mode syncing with OS theme setting mattermost#18140
Has mobile changes MM-23853 Support dark mode and light mode syncing with OS theme setting mattermost-mobile#5635

Screenshots

demo.mp4

Release Note

Added support for dark mode and light mode syncing with OS theme setting

mattermod · 2021-09-08T11:08:31Z

Hello @komik966,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

komik966 · 2021-09-08T11:12:49Z

I'm opening as draft because it requires unit tests addition and e2e tests fixing.

Right after signing in, default theme is used instead one from user preferences. It's known issue to me, I'm going to fix it.

mattermod · 2021-09-19T01:02:08Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

hmhealey · 2021-10-05T14:14:50Z

Hi @komik966. Apologies for the delay in reviews here. I'm not sure why our integration didn't assign reviewers, so this sort of got missed out on

komik966 · 2021-10-05T14:48:18Z

@hmhealey No problem. Recently I'm busy, so I might be slow to respond.

esethna · 2021-10-06T21:58:30Z

@deanwhillier would appreciate your review here considering the theming work you've been doing recently

larkox

LGTM. A few nits here and there. Would be nice to have them solved before merge, but not blocking.

packages/mattermost-redux/src/reducers/entities/general.ts

larkox · 2021-10-13T10:44:45Z

selectors/theme.ts

+import {ThemeKey} from 'mattermost-redux/types/themes';
+
+export const getAllowedThemes = createSelector('getAllowedThemes', getConfig, (config) => {
+    const allowedThemes = (config.AllowedThemes && config.AllowedThemes.split(',')) || [];


Can we simplify this by doing config.AllowedThemes?.split(',') || []?

Fixed. This code was moved from:

mattermost-webapp/components/user_settings/display/user_settings_theme/premade_theme_chooser/index.js

Line 13 in ffdf5b5

const allowedThemes = (config.AllowedThemes && config.AllowedThemes.split(',')) || [];

I think @typescript-eslint/prefer-optional-chain would be useful inside eslint config.

larkox · 2021-10-13T10:46:39Z

selectors/theme.ts

+    const hasAllowedThemes = allowedThemes.length > 1 || (allowedThemes[0] && allowedThemes[0].trim().length > 0);
+    const themesKeys = Object.keys(Preferences.THEMES) as ThemeKey[];
+    return themesKeys.filter((key) => !hasAllowedThemes || allowedThemes.indexOf(key) >= 0).map((key) => Preferences.THEMES[key]);


I am struggling to grasp this logic. Do you mind explaining it to me?

Currently this logic sits here:

mattermost-webapp/components/user_settings/display/user_settings_theme/premade_theme_chooser/premade_theme_chooser.jsx

Lines 18 to 25 in ffdf5b5

const allowedThemes = this.props.allowedThemes;

const hasAllowedThemes = allowedThemes.length > 1 || (allowedThemes[0] && allowedThemes[0].trim().length > 0);

for (const k in Preferences.THEMES) {

if (Preferences.THEMES.hasOwnProperty(k)) {

if (hasAllowedThemes && allowedThemes.indexOf(k) < 0) {

continue;

}

In my implementation filter function requires negation of hasAllowedThemes && allowedThemes.indexOf(k) < 0. I used De Morgan's law (https://en.wikipedia.org/wiki/De_Morgan%27s_laws) to change hasAllowedThemes && allowedThemes.indexOf(k) < 0 -> !hasAllowedThemes || allowedThemes.indexOf(key) >= 0.

components/user_settings/display/user_settings_theme/user_settings_theme.tsx

larkox · 2021-10-13T10:55:07Z

components/user_settings/display/user_settings_theme/expanded/theme_chooser/theme_chooser.tsx

+    return (
+        <>
+            <div className='theme-chooser-header'>{header}</div>
+            <div className='themes-grid'>


Any chance this div may be empty? If so, is there any visual concern that we may want to address? (May be out of the scope of this PR)

larkox · 2021-10-13T11:08:02Z

components/user_settings/display/user_settings_theme/expanded/theme_chooser/theme_chooser.tsx

+                        >
+                            {theme.type?.toLowerCase() === allowedTheme.type?.toLowerCase() && <span className='fa fa-check-circle themes-grid__thumbnail-active-check'/>}
+                            <ThemeThumbnail
+                                themeKey={allowedTheme.type as string}


The problem here is because it may be undefined? Should we instead do something like allowedTheme.type!?

larkox · 2021-10-13T12:11:45Z

components/user_settings/display/user_settings_theme/expanded/index.ts

+
+import {Preferences} from 'mattermost-redux/constants';
+
+import {getAllowedThemes} from '../../../../../selectors/theme';


Can we use a better route?

Should be able to use this:

Suggested change

import {getAllowedThemes} from '../../../../../selectors/theme';

import {getAllowedThemes} from 'selectors/theme';

deanwhillier

Looks really good @komik966! A few things for your consideration. Also, I still need to go through 'components/user_settings/display/user_settings_theme/expanded/expanded.tsx', but thought I'd post what I have for now. Thanks for your work on this and apologies if the recent theme updates added more work for you!

I also have a couple questions to confirm:

What happens when allowed themes only contain light or dark themes?
- With and without custom themes enabled
What happens if allowed themes are changed after an os sync’d theme pair has been selected?

@esethna, FYI for followup consideration, the theme thumbnails need to be made accessible. They were already inaccessible before, so a separate ticket/PR would be good. I also need to tweak the theme SVG thumbnail a bit based on the mobile thumbnail I did.

deanwhillier · 2021-10-13T14:20:46Z

packages/mattermost-redux/src/reducers/entities/general.ts

+    case GeneralTypes.OS_COLOR_SCHEME_INIT:
+    case GeneralTypes.OS_COLOR_SCHEME_CHANGED:


Since ‘OS_COLOR_SCHEME_INIT’ and ‘OS_COLOR_SCHEME_CHANGED’ in essence do the same thing, would it make sense to maybe combine them into one action type using something like ‘SET_OS_COLOR_SCHEME’ or ‘OS_COLOR_SCHEME_SET’ to simplify things a bit?

deanwhillier · 2021-10-13T14:30:41Z

i18n/pl.json

@@ -3353,14 +3353,14 @@
  "user.settings.advance.sendTitle": "Wysyłaj wiadomość przy użyciu Ctrl+Enter",
  "user.settings.advance.sendTitle.mac": "Wysyłaj wiadomość przy użyciu Ctrl+Enter",
  "user.settings.advance.title": "Zaawansowane Ustawienia",
+  "user.settings.custom_theme.advancedThemeEditing": "Zaawansowana edycja motywu",


Only i18n/en.json strings need to be provided, the rest of the language files are auto-generated by the translation build process. If you're interested, feel free to provide these translations using our translation server and get assistance over in our community translation channel.

deanwhillier · 2021-10-13T15:41:58Z

packages/mattermost-redux/src/types/themes.ts

@@ -12,6 +12,7 @@ export type ThemeType = 'Denim' | 'Sapphire' | 'Quartz' | 'Indigo' | 'Onyx';
 export type Theme = {
    [key: string]: string | undefined;
    type?: ThemeType | 'custom';
+    colorScheme?: 'light' | 'dark';


Should we use the OsColorSchemeName type from packages/mattermost-redux/src/types/general.ts here?

deanwhillier · 2021-10-13T18:34:38Z

components/user_settings/display/user_settings_theme/user_settings_theme.tsx

+type Props = {
+    selected: boolean;
+    updateSection: (section: string) => void;
+    setRequireConfirm: (requireConfirm?: boolean) => void;
+    setEnforceFocus: (enforceFocus?: boolean) => void;
+}
+


nit: I'm not sure if we have a standard for this yet or not, but I think the Props type usually lives at the top of component from what I've seen so far.

deanwhillier · 2021-10-13T18:37:12Z

components/user_settings/display/user_settings_theme/user_settings_theme.tsx

+
+const UserSettingsTheme: React.FC<Props> = ({selected, updateSection, setRequireConfirm, setEnforceFocus}: Props) => {
+    return selected ? (
+        <Expanded


Could we maybe use a slightly more descriptive component name here? Something like UserSettingsExpanded?

deanwhillier · 2021-10-13T18:43:56Z

components/user_settings/display/user_settings_theme/expanded/theme_chooser/theme_chooser.tsx

+                            onClick={onCustomThemeClick}
+                        >
+                            {customThemeSelected && <span className='fa fa-check-circle themes-grid__thumbnail-active-check'/>}
+                            <span className='fa fa-pencil'/>


Palette icon is available in compass-icons, our new custom icon font (already available in the web app)
- the following should do the trick with an additional style update to set the font-size to 31.2px (according to the design)

https://mattermost.github.io/compass-icons/

Suggested change

<span className='fa fa-pencil'/>

<i className='icon icon-palette-outline'/>

deanwhillier · 2021-10-13T18:51:38Z

components/user_settings/display/user_settings_theme/expanded/theme_chooser/theme_chooser.tsx

+                            })}
+                            onClick={onCustomThemeClick}
+                        >
+                            {customThemeSelected && <span className='fa fa-check-circle themes-grid__thumbnail-active-check'/>}


We can use our new custom icon font for this now. :)

https://mattermost.github.io/compass-icons/

Suggested change

{customThemeSelected && <span className='fa fa-check-circle themes-grid__thumbnail-active-check'/>}

{customThemeSelected && <i className='icon icon-check-circle themes-grid__thumbnail-active-check'/>}

deanwhillier · 2021-10-13T18:55:46Z

...ts/user_settings/display/user_settings_theme/expanded/theme_chooser/custom_theme_chooser.jsx

+                        />
+                        <div className='header__icon'>
+                            <LocalizedIcon
+                                className='fa fa-plus'


Can use compass-icons icon font here.

https://mattermost.github.io/compass-icons/

deanwhillier · 2021-10-13T18:55:53Z

...ts/user_settings/display/user_settings_theme/expanded/theme_chooser/custom_theme_chooser.jsx

+                                title={{id: t('generic_icons.expand'), defaultMessage: 'Expand Icon'}}
+                            />
+                            <LocalizedIcon
+                                className='fa fa-minus'


Can use compass-icons icon font here.

https://mattermost.github.io/compass-icons/

deanwhillier · 2021-10-13T18:56:57Z

selectors/theme.ts

+
+export const getAllowedThemes = createSelector('getAllowedThemes', getConfig, (config) => {
+    const allowedThemes = (config.AllowedThemes && config.AllowedThemes.split(',')) || [];
+    const hasAllowedThemes = allowedThemes.length > 1 || (allowedThemes[0] && allowedThemes[0].trim().length > 0);


nit: ‘hasAllowedThemes’ seems to be returning undefined instead of false if allowedThemes haven't been defined, maybe switch to ‘??’ Instead of ‘||’?

deanwhillier · 2021-10-13T19:18:26Z

@komik966, I was thinking we should maybe also update setThemeDefaults to add the new colorScheme value to old custom themes that don't have yet have it by defaulting to light? That function is intended to fill in missing values and transform old to new values as needed.

packages/mattermost-redux/src/utils/theme_utils.ts

mattermod · 2021-10-26T01:02:33Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

esethna · 2021-10-26T22:00:17Z

@komik966 let us know if you have questions about Dean's feedback?

komik966 · 2021-10-27T20:37:26Z

@esethna As far no questions. I'll address all comments this weekend.

…os-theme-sync

komik966 · 2021-11-01T21:21:03Z

I will continue covering the comments this week

mattermod · 2021-11-12T01:02:29Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

pat-s · 2021-12-02T19:12:21Z

@komik966 Would love to see this PR getting merged :)

esethna · 2022-02-01T21:50:05Z

Hey @komik966! How's this coming? We're really excited about this feature and would love to get this in!

komik966 · 2022-02-08T07:07:43Z

Hi, @esethna! I didn't make progress here.
But the good news is that I finished some activities which prevented me to continue work on this in the last few months (months! 😮)
So please expect some new changes here till the end of this week.

esethna · 2022-02-08T22:35:28Z

@komik966 that's great news, looking forward to it! Thanks!

I'm going to introduce @laneycs who will be the PM that takes over this project (OS theme syncing). Laney here are the related PRs for server and mobile in addition to this one for web.

…os-theme-sync

esethna · 2022-04-05T19:44:35Z

@komik966 just checking how this is going and if you're still working on it? cc// @laneycs

komik966 · 2022-04-05T20:10:32Z

mattermost/mattermost#15975 (comment)

komik966 added 2 commits September 8, 2021 12:15

MM-23853 Support dark mode and light mode syncing with OS theme setting

977695f

MM-23853-support-os-theme-sync fix tests

fa74efa

mm-cloud-bot added the release-note label Sep 8, 2021

mattermod added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Sep 8, 2021

komik966 marked this pull request as draft September 8, 2021 11:09

MM-23853-support-os-theme-sync fix lint

6a8e931

mattermod added the Lifecycle/1:stale label Sep 19, 2021

hmhealey requested review from larkox and deanwhillier October 5, 2021 14:14

hmhealey removed the Lifecycle/1:stale label Oct 5, 2021

esethna added the Hacktoberfest label Oct 6, 2021

Merge branch 'master' into MM-23853-support-os-theme-sync

fff31be

larkox approved these changes Oct 13, 2021

View reviewed changes

deanwhillier suggested changes Oct 13, 2021

View reviewed changes

mattermod added the Lifecycle/1:stale label Oct 26, 2021

esethna removed the Lifecycle/1:stale label Oct 26, 2021

esethna mentioned this pull request Oct 26, 2021

MM-23853 Support dark mode and light mode syncing with OS theme setting mattermost/mattermost#18140

Closed

esethna requested a review from jgilliam17 October 26, 2021 22:02

Merge remote-tracking branch 'upstream/master' into MM-23853-support-…

f3d5d0e

…os-theme-sync

mattermod added the Lifecycle/1:stale label Nov 12, 2021

esethna closed this Feb 8, 2022

esethna reopened this Feb 8, 2022

esethna requested a review from laneycs February 8, 2022 22:35

Merge remote-tracking branch 'upstream/master' into MM-23853-support-…

797171d

…os-theme-sync

laneycs approved these changes Apr 5, 2022

View reviewed changes

komik966 closed this Apr 5, 2022

	const allowedThemes = this.props.allowedThemes;
	const hasAllowedThemes = allowedThemes.length > 1 \|\| (allowedThemes[0] && allowedThemes[0].trim().length > 0);

	for (const k in Preferences.THEMES) {
	if (Preferences.THEMES.hasOwnProperty(k)) {
	if (hasAllowedThemes && allowedThemes.indexOf(k) < 0) {
	continue;
	}


		import {Preferences} from 'mattermost-redux/constants';

		import {getAllowedThemes} from '../../../../../selectors/theme';

	import {getAllowedThemes} from '../../../../../selectors/theme';
	import {getAllowedThemes} from 'selectors/theme';

		case GeneralTypes.OS_COLOR_SCHEME_INIT:
		case GeneralTypes.OS_COLOR_SCHEME_CHANGED:

	<span className='fa fa-pencil'/>
	<i className='icon icon-palette-outline'/>

	{customThemeSelected && <span className='fa fa-check-circle themes-grid__thumbnail-active-check'/>}
	{customThemeSelected && <i className='icon icon-check-circle themes-grid__thumbnail-active-check'/>}

MM-23853 Support dark mode and light mode syncing with OS theme setting #8804

MM-23853 Support dark mode and light mode syncing with OS theme setting #8804

Conversation

komik966 commented Sep 8, 2021

Summary

Ticket Link

Related Pull Requests

Screenshots

Release Note

mattermod commented Sep 8, 2021

komik966 commented Sep 8, 2021

mattermod commented Sep 19, 2021

hmhealey commented Oct 5, 2021

komik966 commented Oct 5, 2021

esethna commented Oct 6, 2021

larkox left a 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

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

deanwhillier left a comment • edited

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

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

deanwhillier Oct 13, 2021 • edited

Choose a reason for hiding this comment

deanwhillier commented Oct 13, 2021

mattermod commented Oct 26, 2021

esethna commented Oct 26, 2021

komik966 commented Oct 27, 2021

komik966 commented Nov 1, 2021

mattermod commented Nov 12, 2021

pat-s commented Dec 2, 2021

esethna commented Feb 1, 2022

komik966 commented Feb 8, 2022

esethna commented Feb 8, 2022

esethna commented Apr 5, 2022

komik966 commented Apr 5, 2022

deanwhillier left a comment •

edited

deanwhillier Oct 13, 2021 •

edited