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

Adds support for prefers-color-scheme #4380

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

carolinebridge
Copy link
Contributor

@carolinebridge carolinebridge commented May 8, 2024

(Updated with feedback)
Resolves #4173

Adds light | dark | system selector to the preferences panel that determines how themes are rendered.

Themes defined in the config now support either "light" or "dark" options, like so:

"configuration": {
    "theme": {
      "light": {
        "palette": {
          "tertiary": {
            "main": "#9da9b6"
          },
          "secondary": {
            "main": "#29405F"
          }
        }
      },
      "dark": {
        "palette": {
          "mode": "dark",
          "tertiary": {
            "main": "#9da9b6"
          },
          "secondary": {
            "main": "#29405F"
          }
        }
      }
    },
    "extraThemes": {
      "legacy": {
        "name": "Legacy",
        "palette": {
          "primary": {
            "main": "#444"
          },
          "secondary": {
            "main": "#335"
          },
          "tertiary": {
            "main": "#250"
          },
          "quaternary": {
            "main": "#535"
          },
          "mode": "dark"
        }
      },
      "new": {
        "name": "New",
        "light": {
          "palette": {
            "primary": {
              "main": "#F2F3AE"
            },
            "secondary": {
              "main": "#EDD382"
            },
            "tertiary": {
              "main": "#FC9E4F"
            },
            "quaternary": {
              "main": "#F4442E"
            }
          }
        },
        "dark": {
          "palette": {
            "mode": "dark",
            "primary": {
              "main": "#1B065E"
            },
            "secondary": {
              "main": "#FF47DA"
            },
            "tertiary": {
              "main": "#25FF87AB0"
            },
            "quaternary": {
              "main": "#FCC8C2"
            }
          }
        }
      }
    }
  },

The theme, or its alternate, will be used depending on what the user has selected for "mode". The "system" option for mode uses prefers-color-scheme.

If the user selects a theme mode that is not available through the theme's definition (e.g. there is no dark mode defined), the system will generate an "acceptable" theme using the makeContrasting function.

Here's an example with 'stock':

Our theme definition:
image

makeContrasting:
Screenshot 2024-05-17 at 4 42 24 PM

@carolinebridge carolinebridge added the enhancement New feature or request label May 8, 2024
@carolinebridge carolinebridge requested a review from cmdcolin May 8, 2024 13:58
@carolinebridge carolinebridge self-assigned this May 8, 2024
@cmdcolin
Copy link
Collaborator

cmdcolin commented May 8, 2024

I might have to review the code further, but my thoughts on this have been sort of considering a couple ideas

  • we could conceive of themes generally being set up as "paired" so having a light and a dark mode. this is actually visible in our current theme set where there is "lightX" and "darkX" (lightStock and darkStock for example)
  • we could create a system where a single theme can have a light and dark mode. i am just thinking of colorscheme in vim for example where you set a single 'colorscheme' and then it can have light and dark modes.

with the above things the concern "The motivation here is not potentially overriding an admin-configured theme in favour of a JBrowse default theme, on initial load, but we can discuss (could potentially add another config slot that dictates the admin does/does not want the default theme to be the system theme?)" is solved by either

a) the admin specifies a light and dark version of their theme
b) the admins theme has a light and dark mode built in

The themes mixin combs the list of themes in the session for dark/light mode themes (prioritizing user-configured themes) and sets a ‘systemTheme’ as the first match to their configured mode setting

I am a little wary of the process of "combing" through the themes as there is no particular order for the theme list so it seems like it might pick something arbitrary? i would have to check the code

random complaint is that the unfortunate thing is that we cannot just toggle the single switch "mode:dark" for an arbitrary theme dark themes because there are many things that end up being "wrong" with this (that is why you can see a fair amount of custom work is done specifically for dark themes in theme.ts)

@carolinebridge
Copy link
Contributor Author

Define a course of action you think is best for changing some of the ways themes work to integrate this feature in better, if necessary. The way I have it implemented was to change as little as possible.

To determine the "dark/light" theme to apply, it picks the first one in the list of available themes that match the mode, prioritizing the configured themes.

Yeah, being able to just set mode: dark would have made it pretty simple.

@cmdcolin
Copy link
Collaborator

cmdcolin commented May 8, 2024

I don't actually know the best course of action, but there could be some options

a) formalizing the pairing of named themes e.g. "lightX" and "darkX" and auto-select based on the prefers-color-scheme. pros: fairly flexible, works with current name schemes for themes, works with our current concept of a theme. cons: somewhat awkward

b) adding support for a single theme having both light and dark modes, with possibly completely different theme choices in each mode. pros: most flexible, most error tolerant. has some similarities to vim colorschemes. cons: maybe more complex to implement, may change our concept of a theme, might make the naming choice of lightStock and darkStock obsolete, as maybe just one theme name would suffice afterwards (but that could be a pro)

c) toggle the mode:dark switch on the theme.palette in response to prefers-color-scheme. pros: doesn't require that much change, works with our current concept of a theme mostly. cons: vulnerable to problems because it really is hard to anticipate all the changes that changing mode:dark induces, might make the naming choice of lightStock and darkStock obsolete, as maybe just one theme name would suffice afterwards (but that could be a pro)

d) the system proposed in this pr. pros: does not require changing our concept of a theme. cons: the algorithm for selecting a theme is somewhat complex, and may produce unexpected results

e) do nothing. pros: no action needed, doesn't potentially confuse user with why dark theme was auto chosen. i know that i have had trouble with this on my phone: i do not like google maps dark theme for example, but i cant even un-choose it without going into my system preferences. and, indeed, dark themes are actually not really that well baked into jbrowse yet. it needs more work to make the data visualizations look good with dark theme enabled. some labels are not readable for example. cons: doesn't 'solve issue'

i know it would be nice to get an easy win for this issue but i think it is a little tricky :) could discuss in meeting too

@carolinebridge
Copy link
Contributor Author

From meeting:

  • leaning towards option b) presented above
  • possibly apply makeContrasting function to a defined theme if it doesn't have a "sister" theme

e.g. autodetect user's OS mode -> check whether there is a mode: 'dark' equivalent to the currently selected theme -> if not apply makeContrasting to the theme

  • maintain the ability to select other themes that are prioritized over the auto-detect functionality
  • "sister" theme would have to be defined "within" the main theme to some degree to associate the two (this would avoid strict naming requirements but slightly change how the config'd themes are defined)

@cmdcolin
Copy link
Collaborator

we might want to look at MUI v6 concept of themes, i think there are some related ideas https://next.mui.com/material-ui/customization/css-theme-variables/overview/#advantages

@cmdcolin
Copy link
Collaborator

cmdcolin commented May 29, 2024

the concept is actually so similar in this PR and the MUI v6 theme concept that it might be good to just adopt the v6 approach exactly...and then upgrade to v6 when the time comes...suggestions are that it will be soon https://mui.com/blog/2023-material-ui-v6-and-beyond/

@cmdcolin
Copy link
Collaborator

@carolinebridge if you want you can see how jbrowse works with the @mui/material@v6 alpha installed and try out the things that are mentioned in that link

@carolinebridge
Copy link
Contributor Author

Couple of notes from investigation:

  • https://next.mui.com/material-ui/migration/migration-css-theme-variables/ very useful

  • we will need to move away from ThemeProvider to use CssVarsProvider

  • createTheme is being replaced with extendTheme

  • some theme modifications will need to be moved to css style overrides

  • detecting the user's 'mode' is still required (with useMediaQuery) -- this is the 'system' setting and still needs to be stored

  • code referencing 'mode' within packages/core/ui/theme.ts will not be required -- the alternate themes will just need to be placed within the appropriate "dark" and "light" settings in the theme

  • the mode toggle on preferences will use mui's useColorScheme api to change the theme

If we want to merge in this change as-is I think it's fine and there's not really much code that can be changed now to accommodate for what needs to be changed in mui 6.0 -- most of the big changes have to do with ThemeProvider and createTheme being deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: support prefers-color-scheme
2 participants