Fix theme picker text when unspecified theme#42671
Conversation
avatus
added
backport-required
no-changelog
avatus
requested review from
kimlisa,
rudream and
ryanclark
and removed request for
ibeckermayer and
ravicious
ryanclark
left a comment
ryanclark
left a comment
There was a problem hiding this comment.
Can you add some tests for your new helper methods?
| return themePreference === Theme.LIGHT ? lightTheme : darkTheme; | ||
| } | ||
|
|
||
| // because unspecific can exist but only used as a fallback and not an option, |
There was a problem hiding this comment.
| // because unspecific can exist but only used as a fallback and not an option, | |
| // because unspecified can exist but only used as a fallback and not an option, |
| if (currentTheme === Theme.UNSPECIFIED) { | ||
| return getPrefersDark() ? Theme.DARK : Theme.LIGHT; | ||
| } | ||
| } |
There was a problem hiding this comment.
Are you intentionally not returning anything if currentTheme !== Theme.UNSPECIFIED?
| if (currentTheme === Theme.UNSPECIFIED) { | ||
| return getPrefersDark() ? Theme.LIGHT : Theme.DARK; | ||
| } | ||
| return currentTheme === Theme.LIGHT ? Theme.DARK : Theme.LIGHT; |
There was a problem hiding this comment.
Can this be
| if (currentTheme === Theme.UNSPECIFIED) { | |
| return getPrefersDark() ? Theme.LIGHT : Theme.DARK; | |
| } | |
| return currentTheme === Theme.LIGHT ? Theme.DARK : Theme.LIGHT; | |
| return getCurrentTheme(currentTheme) === Theme.LIGHT ? Theme.DARK : Theme.LIGHT; |
|
friendly ping @ryanclark @kimlisa @rudream |
| // and remove the checks for unspecified | ||
| export function getCurrentTheme(currentTheme: Theme): Theme { | ||
| if (currentTheme === Theme.UNSPECIFIED) { | ||
| return getPrefersDark() ? Theme.DARK : Theme.LIGHT; |
There was a problem hiding this comment.
how do i test this? i created a new user, and it defaulted to light theme (using brave)
nvm it worked, not sure what happened before lol.
when i do this flow:
new user -> login -> switch theme
the first click did not work, i had to click a second time to switch the theme
There was a problem hiding this comment.
I wasn't able to reproduce this first click not working in incognito or normal mode
kimlisa
left a comment
kimlisa
left a comment
There was a problem hiding this comment.
just a minor comment, otherwise lgtm
|
friendly ping @ryanclark |
ryanclark
left a comment
ryanclark
left a comment
There was a problem hiding this comment.
Sorry, was waiting for the reply to Lisa's comment 🙂
public-teleport-github-review-bot
Bot
removed the request for review
from rudream
4 participants
Fixes #42395
Currently, we work under the assumption that the theme is a binary decision between light/dark. The default user theme is now
UNSPECIFIEDwhich will default to the system theme. This PR adds a few utility options to get the current/next theme with this unspecified in mind. We can remove them once we add the user settings page to make "use system theme" a valid option to select.To test, you can create a new user in an incognito window. The theme should be whatever your system theme is. Also, the theme selector should know which theme is being used a properly show text for what the next theme should be.