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

feat: Use system default color mode, but allow user override #688

Merged
merged 8 commits into from
May 31, 2023

Conversation

ericvolp12
Copy link
Contributor

@ericvolp12 ericvolp12 commented May 17, 2023

I've implemented a fix for #682 using radio buttons and moved the setting to the Settings page as suggested in the comments below:

chrome_GrxyX238hI
chrome_MeFfIhmSL2

The default is currently system so new users will be using their device default themes when launching the app for the first time but after changing it, will have their preferences stored in the shell state.

OLD:
I've implemented a fix for #682 that doesn't use a radio group but cycles through 3 modes ['light', 'dark', 'system'] and the system mode inherits the device theme properly.

Video Demo:
chrome_2sS6iwAMYw

When system default theme is dark:
image

When system default theme is light:
image

Regardless of the system default theme:
image
image

Tested in mobile browser as well and it inherits an android device theme properly.

Couldn't get the expo test app to work on my phone yet but I did update the Native components so they should expose the same behavior.

This change doesn't change the colorScheme at all, it just updates how the colorScheme is set in ThemeContext.tsx so that we inherit if the user has chosen system as their theme.

We also default in shell.ts to use the system theme to conform to user expectations.

@ericvolp12 ericvolp12 changed the title Feat: Use system default color mode, but allow user override feat: Use system default color mode, but allow user override May 17, 2023
@ansh
Copy link
Contributor

ansh commented May 17, 2023

I have identical code to this for this issue. The problem is a UX one. It's hard to express to users that a singular icon button is a 3-way toggle between light/dark/system on mobile. It's more verbose on web so easier to understand for sure.

@ericvolp12
Copy link
Contributor Author

I took another whack at it using a 3-state radio button, I can't test on mobile at the moment though.

chrome_7JgJpM8pKt

@ericvolp12
Copy link
Contributor Author

Current mobile web looks like:

chrome_veKI1pZg6A

@notdaniel
Copy link

notdaniel commented May 17, 2023

It's hard to express to users that a singular icon button is a 3-way toggle between light/dark/system on mobile.

I think users are used to the word "automatic" in reference to system theme. Paired with the right icon, it would work fine.

So I notice that a moon icon is being used above, even for daytime. If you switched it to a sun icon, then you could use them this way:

  • ☼ Light Theme
  • ☾ Dark Theme
  • ☼/☾ Automatic

I find the text-only buttons to be way less intuitive, personally

@pfrazee
Copy link
Collaborator

pfrazee commented May 17, 2023

I appreciate what you're aiming for here!

Yeah I think the 3-way state is a bit of a challenge. Maybe we move it out of the drawer/right-nav and into the settings?

@pfrazee pfrazee added the x:discussing We've seen the request and we're talking about it! label May 17, 2023
@parion
Copy link

parion commented May 17, 2023

Maybe we move it out of the drawer/right-nav and into the settings?

That's what I was thinking too, something ala this in a Display menu.

image

@ericvolp12
Copy link
Contributor Author

I appreciate what you're aiming for here!

Yeah I think the 3-way state is a bit of a challenge. Maybe we move it out of the drawer/right-nav and into the settings?

I'll take a swing at it after $dayjob, I like that idea too because it lets us put the control in the same place on both web and native.

@snhasani
Copy link

snhasani commented May 17, 2023

@ericvolp12 I cocked these codes last night. But in the end I wasn't happy about the result because of the poor UI and UX. So I didn't create a PR for here but you can check my code and maybe reusing some part of it. I refactor the SelectableButton component and create a new one, GroupButton. And some other stuff.

I think we can put appearance settings into a modal like a few of current settings. Like twitter iOS dark mode settings:

telegram-cloud-photo-size-4-5994759638173727624-y

P.S: Running iOS or Android is a pain in ass. I couldn't run them after a couple of hours of miserable tries. It is too hard to contribute on something that you can not run it :D

cc @pfrazee @ansh

@ericvolp12
Copy link
Contributor Author

Okay, I've moved the appearance 3-way-toggle to the Settings page and removed it from the other components.

chrome_GrxyX238hI
chrome_MeFfIhmSL2

Copy link

@snhasani snhasani left a comment

Choose a reason for hiding this comment

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

Good job! Added a couple of comments.

@@ -189,7 +189,7 @@ export interface ComposerOpts {
}

export class ShellUiModel {
darkMode = false
colorMode = 'system'

Choose a reason for hiding this comment

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

IMO colorMode isn't an intelligible name for something that we call in UI as appearance setting. For me color mode looks like a variable that contains an accent color of the current color scheme. Right now we have only one accent color for each light and dark color scheme. We don't need to have a variable for that now. But in the future when we add it then we don't need to touch this flag. I think appearance is more clear than colorMode in this context.

export const APPEARANCE = {
  SYSTEM: 'system',
  DARK: 'dark',
  LIGHT: 'light',
} as const

type ObjectValues<T> = T[keyof T]

export type AppearanceType = ObjectValues<typeof APPEARANCE>

export class ShellUiModel {
  appearance: AppearanceType
   ...
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get this concern but I don't think it's confusing enough to change

Appearance
</Text>
<View>
<View style={[styles.linkCard, pal.view, styles.selectableBtns]}>

Choose a reason for hiding this comment

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

Here is an opportunity for refactoring and preventing duplicate codes. You can pull out SelectableBtn from src/view/com/modals/ContentFilteringSettings.tsx and create a new one component that reused here and there.

You can use my code from this and this commit. It's already there.

@rezaaa
Copy link
Contributor

rezaaa commented May 19, 2023

Good job guys. I hope you change the component design to something better.

@pfrazee pfrazee merged commit 09ade36 into bluesky-social:main May 31, 2023
@pfrazee
Copy link
Collaborator

pfrazee commented May 31, 2023

Awesome, thanks everybody

@ansh ansh removed the x:discussing We've seen the request and we're talking about it! label May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants