Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

Add support for light/dark themes #10285

Closed
wants to merge 1 commit into from

Conversation

anler
Copy link

@anler anler commented May 8, 2022

Summary

In this PR I'm breaking all rules of good code and abstraction, so it's only intended as an icebreaker in the support of themes with light/dark variants. I'm a heavy user of that since depending on the amount of light I have in my room I prefer to code with one or the other.

I first tried to use media queries until I found that css-var-ponyfill has no plan to support that at the moment: jhildenbiddle/css-vars-ponyfill#31 (comment)

Ticket Link

N/A

Screenshots

Selecting the "System" theme it will automatically toggle between Denim (light) and Onyx (dark)

Think of the "System" theme as having this icon (didn't have the time to experiment with that):

CleanShot 2022-05-08 at 18 47 20@2x

CleanShot.2022-05-08.at.18.42.04.mp4

@mm-cloud-bot
Copy link

@anler: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mattermod
Copy link
Contributor

Hello @anler,

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.

@mattermod mattermod requested review from a team and devinbinnie and removed request for a team May 8, 2022 16:44
@mattermod mattermod added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Contributor labels May 8, 2022
@AshishDhama AshishDhama added 1: PM Review Requires review by a product manager 1: UX Review Requires review by a UX Designer labels May 9, 2022
@AshishDhama AshishDhama requested review from AshishDhama and michelengelen and removed request for devinbinnie May 9, 2022 07:41
Copy link
Contributor

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

LGTM

@AshishDhama
Copy link
Contributor

Ticket for this mattermost/mattermost#15975

@matthewbirtch
Copy link
Contributor

matthewbirtch commented May 9, 2022

Sorry @anler. This solution conflicts with the proposed experience for syncing themes with the OS: https://mattermost.atlassian.net/browse/MM-23853

I believe work on this was started, but it looks like it might have been abandoned: mattermost/mattermost#15975

cc @esethna

Copy link
Contributor

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

revoking my approval since i was missing the information from @matthewbirtch ...

@esethna
Copy link
Contributor

esethna commented May 9, 2022

@anler really appreciate your work here.

Matt shared there is a ticket already outstanding for this work, so in order to take this change it would have to be designed to that spec.

The other complexity here is that we are soon starting a revamp of our account settings panel: https://mattermost.atlassian.net/wiki/spaces/GLOAB/pages/2281046017/Settings+Revamp. The changes here will conflict with this.

I propose we close this PR in advance of that work and revisit adding the theme syncing after that work is complete.

@esethna esethna closed this May 9, 2022
@anler
Copy link
Author

anler commented May 9, 2022

Hi @matthewbirtch no worries, I wasn't expecting the PR to be merged, not at least in its current state 😄 Just wanted to find out if this was something we wanted to have and with which degree of complexity and I see you opted for the most flexible but laborious solution.

If mattermost/mattermost#15975 is abondoned I can try go through it and make another proposal.

@anler anler deleted the light-dark-themes branch May 9, 2022 17:51
@Willyfrog
Copy link
Contributor

Hey @anler good to see you around here!!

The ticket is marked as up for grabs so yes, you can go through the proposal and request to give it a go. Also a previous contributor created a bunch of PRs (now closed) that might help you get started if you like.

Comment in mattermost/mattermost#15975 that you want it assigned (otherwise github makes it really hard to assign issues to people) and you are good to go.

Also, thanks for this PR even if it wasn't merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1: PM Review Requires review by a product manager 1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester Contributor do-not-merge/release-note-label-needed
Projects
None yet
8 participants