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

Support dark mode and light mode syncing with OS theme setting #15975

Open
mattermod opened this issue Oct 15, 2020 · 40 comments
Open

Support dark mode and light mode syncing with OS theme setting #15975

mattermod opened this issue Oct 15, 2020 · 40 comments

Comments

@mattermod
Copy link
Contributor

mattermod commented Oct 15, 2020

Summary

  • Introduces a new setting "Sync with OS appearance settings"
    • If this setting is on, it will sync the user’s theme with the OS dark/light mode settings with the selected dark and light themes
    • If this setting is off, it will always use the one selected theme regardless of the OS settings
  • Existing themes will need to now be defined as light or dark in the theme definition somehow
    • Ensure there are no breaking changes for users using existing themes
    • Will need a solution to migrate custom themes
  • Group the ‘Copy Theme Colors’ function and JSON code in to another collapsable element within the Display settings called ‘Advanced Theme Editing’
    • “See other themes” and “Import theme colors from Slack” has also been moved to this section
  • Dark and Light Themes can each be Custom themes - so users will now be able have two custom themes when Sync is turned on and each will have its own set of collapsable rows for customization.
  • Detailed notes in Figma Design File

image

Other things to consider

  • If the browser doesn’t support syncing with OS settings, don’t show this setting
  • What limitations might there be for Windows/Android? Which version of the OS supports this and therefore which should we set the default to TRUE for?
  • Custom theme generator: https://avasconcelos114.github.io/mattermost-themes/
    • already has light and dark themes categorized

Prototypes

Design File

Technical implementation notes:

  • When renaming the themes, we'll only change the display name, not the internal name. That's already supported since we renamed the themes in the past at some point ("Default" -> "Mattermost", "Windows 10" -> "Windows Dark")
  • Internally, the themes should be stored in preferences named "theme" (the light theme) and "dark_theme" since this makes compatibility much easier between
  • Syncing with the OS theme will be account-wide. If the user wants to turn off light/dark theme on a specific device, they'd likely disable the automatic switching of OS themes on that device.
  • Syncing with the OS theme will apply to all teams.
  • Is this available on web? What does the desktop app require for this? This might work:

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-23853

@kishore007k
Copy link

I would like to work on this issue.

@esethna
Copy link
Contributor

esethna commented Jan 11, 2021

Hi @kishore007k, happy new year! Are you still working on this issue or should I reopen it for someone else?

@kishore007k
Copy link

hi @esethna I was working on this issue but now I've got my semester exams going on and it might take some time. So i can't working on this

@esethna
Copy link
Contributor

esethna commented Jan 12, 2021

No problem @kishore007k, I will reopen the feature up for grabs

@komik966
Copy link

komik966 commented Jun 3, 2021

I have some considerations according this issue and would know your opinion.

Existing users who never changed theme

Figma file states:

Default setting for ‘Sync with OS appearance’:

  • existing users => OFF (retain existing theme)
  • new users => ON

Can we ease this rule to this:

Default setting for ‘Sync with OS appearance’:
- users who changed theme (has record in preferences table with category='theme' and name='') => OFF (retain existing theme)
- users who didn't change theme => ON

?

Not supported devices

When user has allowed theme sync, but opens application on other device which doesn't support it - how should we fallback? Use light theme?

Store users' theme preferences

Currently, theme is stored as serialized json in preferences table with category='theme' and name=''
This should stay as is - this record will be used when theme sync is off.

I would add these new preferences "types":

  • category='theme' and name='light' - store serialized light theme (when user switches theme sync off, not remove this record - it'll be used when user switches on again)
  • category='theme' and name='dark' - store serialized dark theme (when user switches theme sync off, not remove this record - it'll be used when user switches on again)
  • category='theme' and name='sync_with_os' - true/false

Detect if theme is light or dark

When user has custom theme and is switching sync on, we need to detect if theme is light or dark.

My proposition:
Convert theme's centerChannelBg to HSL (https://en.wikipedia.org/wiki/HSL_and_HSV). If L > 50 then theme is light else dark

@esethna
Copy link
Contributor

esethna commented Jun 3, 2021

Can we ease this rule to this:

Default setting for ‘Sync with OS appearance’:

  • users who changed theme (has record in preferences table with category='theme' and name='') => OFF (retain existing theme)
  • users who didn't change theme => ON

@komik966 yes if we can reliably know if a user has edited their theme then I think that's a great proposal. @hmhealey might have thoughts here

When user has allowed theme sync, but opens application on other device which doesn't support it - how should we fallback? Use light theme?

Yes I think light theme is a good fallback

I would add these new preferences "types":

I will refer to @hmhealey or @devinbinnie on this one, but your proposal sounds reasonable

When user has custom theme and is switching sync on, we need to detect if theme is light or dark.

My proposition:
Convert theme's centerChannelBg to HSL (https://en.wikipedia.org/wiki/HSL_and_HSV). If L > 50 then theme is light else dark

Sounds reasonable to me unless dev has objections.

@komik966 would you like me to assign this ticket to you! Thanks for the interest!

@komik966
Copy link

komik966 commented Jun 3, 2021

Yes, please assign me to this ticket

@komik966
Copy link

komik966 commented Jun 4, 2021

I'm rethinking how to store theme preferences and I'm seeing some defects in my proposal. I'll describe my thoughts soon.

@hmhealey
Copy link
Member

hmhealey commented Jun 7, 2021

I'm not sure if this is what you were trying to fix, @komik966, but the proposal for storing the themes wouldn't quite work since we use the name field to provide a team ID for team-specific themes.

If we're defaulting to the light theme though for unsupported devices, we could continue to use the existing names for light themes and add dark_ for dark ones, so you'd end up with up to 4 theme preferences per team like:

  • name="" - Global light theme which is used if team-specific light theme isn't set. It's also used if global dark theme isn't set or dark mode isn't supported by device.
  • name="ui6wy4q7rpyb5mp7dhzn4w3jir" - Team-specific light theme. It's also used if dark theme isn't set or dark mode isn't supported.
  • name="dark" - Global dark theme which is used if team-specific dark theme isn't set.
  • name="dark_ui6wy4q7rpyb5mp7dhzn4w3jir" - Team-specific dark theme.
    And then that one setting for whether or not to sync dark mode with the OS.

Alternatively, we could have two categories theme and theme_dark and keep the name as the team ID like we currently have. We'd need to find somewhere for the dark mode sync setting to go then, but that could end up being a bit cleaner since we aren't having to parse the name any more. I could see either of those methods working just fine.

@komik966
Copy link

komik966 commented Jun 8, 2021

When writing first proposal, I wasn't aware of team-specific themes.

If we continue to use theme category we will not be able to detect if this theme was saved before introduction of os synchronization feature (if it was, we should not synchronize with os).

My proposition is to deprecate theme category and add new one: theme_os_synchronizable.
Preference's Name will be optional team ID (as it works now for theme)
Preference's Value will be serialized json in form of:

type themePreference struct {
	SynchronizeWithOs bool              `json:"synchronize_with_os"`
	Dark              map[string]string `json:"dark,omitempty"`
	Light             map[string]string `json:"light,omitempty"`
	NotSynchronized   map[string]string `json:"not_synchronized,omitempty"`
}

example value:

{
  "synchronizeWithOs": true,
   "dark": { sidebarBg: "#1b2c3e", ... }
}

It feels easier to maintain one "atomic" theme preference.

@esethna
Copy link
Contributor

esethna commented Jun 8, 2021

@hmhealey on the above if you have thoughts?

@hmhealey
Copy link
Member

I'd be worried that combining those all together would unfortunately cause problems. Working with a JSON object in Preferences can be tricky, particularly if we ever have to do a database migration since it's hard to work with JSON in the dataabase, and I'd also be worried that we might run out of space in the database column by putting all the themes together. Also, for compatibility, we'd need to maintain the old theme preference in parallel with this one, so I still think it might be easier to have theme be one of the light or dark themes instead.

I like the idea of being able to update them atomically, but I think we can mostly achieve that even if they're in separate preferences. We already have to update multiple preferences at once with the current system for the team-specific ones, so we could continue to use that API to store the dark/light themes at the same time.

As for storing the unsynchronized theme, I don't think we actually need to store it separately. When the user enables syncing, we can figure out if the unsynced theme is dark or light as you've suggested, and when the user disables syncing, I think we can just pick one of the themes and use that. If the user wants something different, they can change it before saving and closing their settings. The only time the unsynchronized theme will matter when the setting is enabled is if they're using an older client that doesn't support it, but I think it's still fine to just pick one of the light/dark themes (which will be the one stored in the theme preference for backwards compatibility).

@komik966
Copy link

@hmhealey Agree that json in db may cause problems.
How should we store information if sync is enabled?
I was thinking about two options:

  1. add preference category disable_theme_sync - sync is enabled if user hasn't this preference or is set to false
    pros:
    ✅ every user has enabled synchronization by default, no need to insert additional records to db
    cons:
    ⛔️ disable_theme_sync should be added to current users who have theme (some kind of data migration needed)
  2. add preference category enable_theme_sync and set it to true for new users (during registration)
    pros:
    ✅ theme behavior will remain unchanged for existing users who changed theme before
    cons:
    ⛔️existing users who never changed theme will need to allow sync explicitly

@komik966
Copy link

komik966 commented Jun 10, 2021

Or maybe third option, without adding additional preference category:
theme exists => not sync, use this theme
theme and theme_dark exists => sync, use these themes
theme not exists and theme_dark not exists => sync, use default themes
pros:
✅ every user has enabled synchronization by default, no need to insert additional records to db
✅ theme behavior will remain unchanged for existing users who changed theme before
cons:
⛔️if user wants to change only light theme, but still sync with os - we will also need to save theme_dark with default value

🤔

@esethna
Copy link
Contributor

esethna commented Jul 22, 2021

Thanks @komik966. My thought is that it's fine as is to start and we can improve that separate from this scope of work.

Excited that you've picked this back up, let us know if we can help. This is such a high impact change!

@komik966
Copy link

I think changes for the mobile app are ready (I need to fix & write some tests)
demo
Now I'm working on webapp.

@esethna
Copy link
Contributor

esethna commented Aug 3, 2021

@komik966 nice!!!

@komik966
Copy link

komik966 commented Aug 12, 2021

scaled.mp4

Webapp is implemented. Now, I'm testing edge cases and need to make minor fixes. Above screen recording shows how it works: theme switching, form synchronization across devices, modal window closing confirmation when we have unsaved changes.

Things worth noticing:

  • "Custom theme" tile has a pencil icon instead of a palette icon because it is not available in Font Awesome version we're using (according to my research)
  • Because more themes are available than Figma design assumes, "Custom theme" tile overflows to the second row when os sync is off.
  • In the mobile app, themes have other names. For that reason, themes selected in webapp are not marked as selected in mobile
  • "Sync with OS appearance" setting is team-wide, not application-wide.
    Rationale: when switching os sync off => on, webapp categorizes the current theme as dark or light and sets the default theme for not categorized color scheme. If this setting was application-wide, such categorization would be done under the hood for every team. It is harder to implement and it leads to saving preferences' changes which user is not aware of.

@esethna
Copy link
Contributor

esethna commented Aug 12, 2021

@komik966 this is phenomenal work, SO stoked for this!

@esethna
Copy link
Contributor

esethna commented Aug 12, 2021

The mobile app themes will be updated to match webapp with this ticket: https://mattermost.atlassian.net/browse/MM-37553 (not started yet)

@esethna
Copy link
Contributor

esethna commented Sep 2, 2021

@komik966 will you be submitting a webapp PR for this as well? Let us know so we can consolidate our testing efforts cc// @jgilliam17 on the server and mobile PRs already submitted ^

@komik966
Copy link

komik966 commented Sep 2, 2021

@esethna Yes, I'll. But first I need to make requested changes to server because it results in changes to the webapp.

@esethna
Copy link
Contributor

esethna commented Sep 2, 2021

Cool. @komik966 as long as the branch names are the same on the webapp and server PRs our test servers will pick up the changes together. We usually test the server and webapp changes together for features, so let us know if that works for you?

@komik966
Copy link

komik966 commented Apr 5, 2022

Hi @laneycs, @esethna
In the near future, I will not be able to spend the required time to finish this feature.
I'm unassigning myself from this issue and closing related PRs to give someone else the opportunity to work on this.

If you find it useful, feel free to use the work I've done so far. I hope that it will contribute to easier delivery of this feature :)

@komik966 komik966 removed their assignment Apr 5, 2022
@esethna
Copy link
Contributor

esethna commented Apr 5, 2022

Thanks @komik966 for the work done so far, we really appreciate it. I hope we can continue this project soon as it brings so much benefit to users to sync themes with OS settings, especially on mobile! cc// @laneycs

@fleytman
Copy link

Is there any news about supporting the synchronization of the theme with the operating system?

@esethna
Copy link
Contributor

esethna commented Oct 17, 2022

Closing this issue as it is being worked on right now internally by @AshishDhama (for webapp/desktop), we may reopen the mobile tickets after that's complete

@esethna esethna closed this as completed Oct 17, 2022
@AshishDhama AshishDhama self-assigned this Oct 20, 2022
@EtzBetz
Copy link

EtzBetz commented Mar 12, 2023

So, there are several issues and tickets floating around about syncing dark/light mode with the os theme, but they all seem closed atm, with https://mattermost.atlassian.net/wiki/spaces/GLOAB/pages/2281046017/Settings+Revamp being said being the offender here for blocking this issue.
Is there any news about this issue? Do we really have to wait for that revamp to add this (in my opinion quite fundamental) feature? Mattermost is my only daily application as of today, which does not support auto theme switching.
Is there anything I can do to help out (without that work being thrown in the bin (not meant rude)) for this to progress faster?

@AshishDhama
Copy link
Collaborator

@EtzBetz Thanks for your patience. Some bigger changes are happening right now, focusing on mono-repo, which will affect a lot of open PRs, and then there is focus on quality. That being said, there is another big effort going on to revamp UX of all the Modals, and this will be revamped with that effort, mostly next month. You will see this shipped with our 8.0 release if everything goes as it says in the plan.
And yeah, we are always open to contributions, so please feel free to open a pr and ping us if you need any help.

@larkox larkox reopened this Aug 9, 2023
@larkox
Copy link
Contributor

larkox commented Aug 9, 2023

This ticket should be ready again for Help Wanted.

@EtzBetz
Copy link

EtzBetz commented Jan 12, 2024

@EtzBetz Thanks for your patience. Some bigger changes are happening right now, focusing on mono-repo, which will affect a lot of open PRs, and then there is focus on quality. That being said, there is another big effort going on to revamp UX of all the Modals, and this will be revamped with that effort, mostly next month. You will see this shipped with our 8.0 release if everything goes as it says in the plan. And yeah, we are always open to contributions, so please feel free to open a pr and ping us if you need any help.

Have these changes been made at this point? Is it safe to work on this feature without work getting thrown into the bin? And also, where do I start? I'm familiar with web dev etc., but mostly with angular and vue, not react. Could you give me a list of pointers what would need to be done?

@jordanwmcdonald
Copy link

I would sincerely appreciate this feature.

@colemanw
Copy link

@afroeagle I couldn't wait for this feature so I installed the DarkReader browser extension on Chrome. It works nicely (even when Mattermost is in a chrome App).

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

No branches or pull requests