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

Veggie Seasons UI doesn't handle dark mode well #322

Closed
RedBrogdon opened this issue Feb 7, 2020 · 6 comments
Closed

Veggie Seasons UI doesn't handle dark mode well #322

RedBrogdon opened this issue Feb 7, 2020 · 6 comments
Labels
bug Something isn't working p1
Milestone

Comments

@RedBrogdon
Copy link
Contributor

Title says it all. We need to update the app to deal with iOS's dark mode properly rather than displaying black-on-black text in several places within the app.

@abd99
Copy link
Contributor

abd99 commented Mar 23, 2020

I have started working on the fix for this issue.
I am planning on changing the declared styles to a method and passing it the BuildContext as a parameter.

Eg:

static const headlineName = TextStyle(
    color: Color.fromRGBO(0, 0, 0, 0.9),
    fontFamily: 'NotoSans',
    fontSize: 24,
    fontStyle: FontStyle.normal,
    fontWeight: FontWeight.bold,
  );

Will turn into:

static TextStyle headlineName(BuildContext context) => TextStyle(
        color: CupertinoTheme.of(context).textTheme.textStyle.color,
        fontFamily: 'NotoSans',
        fontSize: 24,
        fontStyle: FontStyle.normal,
        fontWeight: FontWeight.bold,
      );

Do you think this is a good solution? Or is there a better solution?
Please let me know @RedBrogdon.

@RedBrogdon
Copy link
Contributor Author

RedBrogdon commented Mar 25, 2020

To be quite honest, the way I originally set up the fonts and colors in Veggieseasons was not a great architecture. A lot of the support for theming in Cupertino didn't exist 12-16 months ago when the app was written.

Now, though, it should be possible to use CupertinoTextThemeData:

https://github.com/flutter/flutter/blob/210f4d83136a2eae773bda471db90ac27676f662/packages/flutter/lib/src/cupertino/text_theme.dart#L110

And CupertinoThemeData:

https://github.com/flutter/flutter/blob/210f4d83136a2eae773bda471db90ac27676f662/packages/flutter/lib/src/cupertino/theme.dart#L146

to do the job right.

If you look at an app like the Material Gallery, rather than having a file that sets up a bunch of font/color constants, it has a file that constructs its ThemeData objects for light and dark theme:

https://github.com/flutter/gallery/blob/master/lib/themes/gallery_theme_data.dart

This app could do the same thing, and set up some base text styles that work in a number of places, and then modify them as needed in particular widgets (to tweak font weight or whatever). There would still be some colors, gradients, and so on remaining in that styles file, but a lot of the text styling could be pulled out and "done right," so to speak.

So, while the change you're suggesting would work, there's an opportunity here to improve the app in a more fundamental way. Since these apps are meant to serve as examples of the best ways to do things with Flutter, that's really where we should be aiming.

If that's more work than you were looking to contribute, I completely understand, but I do feel it's the right way to go here.

@RedBrogdon
Copy link
Contributor Author

Some further context:

#395 has been merged, which adds better dark mode handling to VeggieSeasons (thanks, @abd99!). It's a good solution for now, but it would still be better to implement the styling in a manner more in keeping with current SDK capabilities.

@ADVavvas
Copy link
Contributor

ADVavvas commented Nov 8, 2020

Hi,

I'd like to try this.

Quick question:
The Material Gallery app uses ThemeData which in turn uses ColorScheme. CupertinoThemeData does not. Should I stick with CupertinoThemeData or use ThemeData instead?

Thanks

@tusharojha
Copy link
Contributor

@RedBrogdon, I think we can close this issue now.

@RedBrogdon
Copy link
Contributor Author

I believe so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p1
Projects
None yet
Development

No branches or pull requests

4 participants