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

Replace ThemeBloc with ThemeSettings class to change ThemeBrightness, textScalingFactor and VisualDensity. #158

Merged
merged 15 commits into from
May 5, 2022

Conversation

Jonas-Sander
Copy link
Collaborator

@Jonas-Sander Jonas-Sander commented May 4, 2022

Replaced ThemeBloc with a new class called ThemeSettings that can be used to:

  • Toggle between dark-mode, light-mode and using the system setting
  • Change the VisualDensity of the app
  • Change the textScalingFactor

Currently the user can still only toggle the brightness (e.g. dark-mode).
Another PR in the Future might add a nicer UI to change also the other settings.

The ThemeSettings class is used via Provider and doesn't need a StreamProvider:

 @override
 Widget build(BuildContext context) {
   // Will automatically rebuild if properties of [ThemeSettings] change.
   // See also `context.select` to only rebuild if the brightness changes.
   final themeSettings = context.watch<ThemeSettings>();

   return Column(
     children: [
       Text('Current brightness: ${themeSettings.themeBrightness}'),
       ElevatedButton(
         child: Text('Change to dark-mode'),
         onPressed: () => themeSettings.themeBrightness = ThemeBrightness.dark,
       )
     ],
   );
 }

@github-actions
Copy link

github-actions bot commented May 4, 2022

Visit the preview URL for this PR (updated for commit e6c1d6c):

https://sharezone-test--pr158-theme-settings-eo83ht8t.web.app

(expires Thu, 12 May 2022 12:32:31 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@Jonas-Sander Jonas-Sander marked this pull request as ready for review May 4, 2022 16:55
@Jonas-Sander Jonas-Sander changed the title Add ThemeSettings(Notifier) class to change ThemeBrightness, textScalingFactor and VisualDensity Replace ThemeBloc with ThemeSettings class to change ThemeBrightness, textScalingFactor and VisualDensity. May 4, 2022
@Jonas-Sander
Copy link
Collaborator Author

This is how it looks like if you change

textScalingFactor

TextScalingFactor.mov

VisualDensity

Visual.Density.mov

Copy link
Member

@nilsreichardt nilsreichardt left a comment

Choose a reason for hiding this comment

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

Nice! 🚀
LGTM

Can you add tests for ThemeSettings?

And we should also have Analytics, when someone changes the theme. Should we do this in another PR?

app/lib/account/theme/theme_settings.dart Show resolved Hide resolved
app/lib/account/theme/theme_settings.dart Outdated Show resolved Hide resolved
app/lib/account/theme/theme_settings.dart Show resolved Hide resolved
app/lib/account/theme/theme_settings.dart Outdated Show resolved Hide resolved
Comment on lines +153 to +173
extension on ThemeBrightness {
String serialize() {
return {
ThemeBrightness.dark: 'dark',
ThemeBrightness.light: 'light',
ThemeBrightness.system: 'system',
}[this]!;
}
}

extension on String? {
ThemeBrightness? toThemeBrightness() {
if (this == null) return null;

return {
'dark': ThemeBrightness.dark,
'light': ThemeBrightness.light,
'system': ThemeBrightness.system,
}[this];
}
}
Copy link
Member

Choose a reason for hiding this comment

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

When #153 is merged, we can use the built in Dart features for this (released in 2.15):

enum MyEnum {
  one, two, three
}

void main() {
  print(MyEnum.one.name);  // Prints "one".
}
print(MyEnum.values.byName('two') == MyEnum.two);  // Prints "true".

https://medium.com/dartlang/dart-2-15-7e7a598e508a

@Jonas-Sander Jonas-Sander requested a review from nilsreichardt May 5, 2022 12:27
Copy link
Member

@nilsreichardt nilsreichardt left a comment

Choose a reason for hiding this comment

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

LGTM

@Jonas-Sander Jonas-Sander merged commit 97cc53d into main May 5, 2022
@Jonas-Sander Jonas-Sander deleted the theme-settings branch May 5, 2022 14:33
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.

2 participants