Styling revamp using CupertinoThemeData and CupertinoTextThemeData #593

ADVavvas · 2020-11-12T13:03:39Z

This was done to handle styling in a manner more in keeping with current SDK capabilities. (Issue #322 )

… handle dark mode

redbrogdon

Thanks for the PR, @ADVavvas. Has this been tested with both light and dark mode?

ADVavvas · 2020-11-15T13:54:43Z

Hi, forgot to include an important addition to my PR. Should be working fine now for both light and dark mode.

There are still 2 issues with the light/dark theme (although I'm not too sure how to deal with them).

Changing IOS theme while the app is running doesn't update the "In Season Today" headline color (Probably because the widget is not rebuilding?)
On the 'Home' and 'Search' tabs the status bar color (time, wifi, battery) doesn't change (if you change theme while the app is running). It does change if you go to 'My Garden' or 'Settings' (probably because they have a NavigationBar)

redbrogdon

Thanks, @ADVavvas. This PR looks pretty good. I just have a few comments, which are inline.

For #2, I am able to replicate the issue, but I'm not sure whether it's something wrong with this app or an SDK bug. @LongCatIsLooong, would you mind taking a look? Steps to reproduce are:

Launch Veggieseasons in light mode.
Switch to the "My Garden" screen using the bottom navigation bar.
Switch back to the "Home" screen.
Change from light mode to dark mode using iOS developer settings.
Return to the app, and the status bar across the top will have no visible text for time, battery, etc.
Switch to the "My Garden" screen, and white text will appear in the status bar.
Return to the "Home" screen, and the white text will remain.

This is on an iPhone SE Simulator running iOS 14, with this flutter doctor output:

[✓] Flutter (Channel master, 1.24.0-8.0.pre.267, on Mac OS X 10.15.6 19G2021 darwin-x64, locale en-US)
[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
[✓] Xcode - develop for iOS and macOS (Xcode 12.0)
[✓] Chrome - develop for the web
[✓] Android Studio (version 4.0)
[✓] IntelliJ IDEA Community Edition (version 2020.1.2)
[✓] VS Code (version 1.50.1)
[✓] Connected device (4 available)

Is that WAI and there's something we're doing wrong in this app, or is the SDK not behaving itself?

experimental/veggieseasons/lib/screens/details.dart

experimental/veggieseasons/lib/screens/list.dart

experimental/veggieseasons/lib/widgets/trivia.dart

LongCatIsLooong · 2020-11-17T18:10:19Z

experimental/veggieseasons/lib/screens/details.dart


        return Text(
          '$percent% DV',
+          style: CupertinoTheme.of(context).textTheme.textStyle,


nit: is it needed to explicitly specify the text style? I'd expect the Text widget to inherit the style from the theme.

Perhaps I've done something wrong in assigning the theme to the whole app, but removing the style makes the text black regardless of the IOS brightness.

Does it work if you wrap the DetailsScreen widget in a DefaultTextStyle(style: CupertinoTheme.of(context).textTheme.textStyle) widget? Anyways not a big deal it just makes the code a little bit more verbose.

LongCatIsLooong · 2020-11-17T18:25:30Z

experimental/veggieseasons/lib/styles.dart

-  );
+  static TextStyle minorText(CupertinoThemeData themeData) =>
+      themeData.textTheme.textStyle.copyWith(
+        color: Color.fromRGBO(128, 128, 128, 1),


It seems like we're using a regular color. Is "minorText" supposed to use the same color when you switch from light mode to dark mode? Same with "cardTitleText" and "cardCategoryText", etc,.

minorText is used for the date in the Home Screen. It looks okay with both themes since it's grey.

The cards are all white with black text, which also works fine since they have a picture (and shadows).
I haven't changed any of these values. I assume it was intentional.

I see. The cards all have very bright backgrounds because of the images they used. Make sense to opt-out of dark mode on the cards.

LongCatIsLooong · 2020-11-17T18:38:08Z

experimental/veggieseasons/lib/screens/details.dart

+                      1: Text(
+                        'Trivia',
+                      )
+                      //style: CupertinoTheme.of(context).textTheme.textStyle),


I vaguely remember CupertinoSegmentedControl doesn't work well with dark mode, because the UI component no longer exists on iOS 13 and above.

Seems okay on IOS 13.1.
Blue background with black letters for selected. Black background with blue letters for unselected.
Have not applied any styling to the its children.
Not sure what it's supposed to look like.

On iOS 13+ UISegmentedControls have a different look so I'm not sure either what it is supposed to look like. I'm okay with this since there's no actual code change (could you remove the lines that are commented out?)

ADVavvas · 2020-11-19T15:38:43Z

That should be all the changes (except #2).

Thanks you both! Appreciate the feedback.
Let me know if there's anything else.

redbrogdon

This LGTM. @LongCatIsLooong, WDYT?

experimental/veggieseasons/lib/widgets/veggie_card.dart

experimental/veggieseasons/lib/styles.dart

LongCatIsLooong

LGTM with nits.

LongCatIsLooong · 2020-11-20T07:46:52Z

Ah didn't notice this

On the 'Home' and 'Search' tabs the status bar color (time, wifi, battery) doesn't change (if you change theme while the app is running). It does change if you go to 'My Garden' or 'Settings' (probably because they have a NavigationBar)

~~I thought this bug has long been fixed, but the status bar icons are not visible on my android device either. Taking a look.~~

The system chrome logic lives in the CupertinoNavigationBar class so it does not work if the page doesn't have a nav bar (such is the case of the home page and the search page). I guess we can manually add an AnnotatedRegion<SystemUiOverlayStyle> to the tree.

…t screens.

ADVavvas · 2020-11-21T22:39:55Z

Added the AnnotatedRegion<SystemUiOverlayStyle> on both list.dart and search.dart. Not sure where exactly to place it in the tree, so I'd appreaciate if you could take a look @LongCatIsLooong . Works fine though. Also removed the unnecessary imports.

LongCatIsLooong · 2020-11-23T19:45:50Z

experimental/veggieseasons/lib/screens/list.dart

-            },
+        return AnnotatedRegion<SystemUiOverlayStyle>(
+          value:
+              SystemUiOverlayStyle(statusBarBrightness: themeData.brightness),


CupertinoThemeData.brightness is the brightness override and it's nullable. I think this should be MediaQuerty.brightnessOf(context). Same with the search page.

Done! Ignore the first commit (wrote MediaQuery.of(context).platformBrightness instead. Not sure if that makes any difference)

IIRC they're the same.

…ormBrightnessOf)

LongCatIsLooong

Thank you!

ADVavvas · 2020-11-26T13:46:15Z

Quick question since this is my 1st PR: Do I need to do anything else or is it going to be merged at some point?

LongCatIsLooong · 2020-12-01T19:44:08Z

@ADVavvas sorry for the late reply. I'm not particularly familiar with the samples repo procedures. @redbrogdon is it good to merge this PR?

redbrogdon · 2020-12-02T07:04:23Z

Yes, this is ready to land. I would have done it last week, but was out for Thanksgiving. 🦃

redbrogdon · 2020-12-02T07:05:23Z

@ADVavvas, thanks for sticking with this PR through all the review. It's definitely an improvement. I wrote that original text styling two years ago -- it was seriously out of date, but I had no time to fix it. 😄

Styling revamp using CupertinoThemeData and CupertinoTextThemeData to…

d9da034

… handle dark mode

redbrogdon reviewed Nov 15, 2020

View reviewed changes

Added TextStyle to all texts

bf79045

redbrogdon requested changes Nov 17, 2020

View reviewed changes

experimental/veggieseasons/lib/screens/details.dart Outdated Show resolved Hide resolved

experimental/veggieseasons/lib/screens/list.dart Outdated Show resolved Hide resolved

experimental/veggieseasons/lib/widgets/trivia.dart Show resolved Hide resolved

LongCatIsLooong reviewed Nov 17, 2020

View reviewed changes

Removed comments. Changed trivia score text. Fixed In Season text bug

5e39732

redbrogdon approved these changes Nov 20, 2020

View reviewed changes

LongCatIsLooong reviewed Nov 20, 2020

View reviewed changes

experimental/veggieseasons/lib/widgets/veggie_card.dart Outdated Show resolved Hide resolved

LongCatIsLooong reviewed Nov 20, 2020

View reviewed changes

experimental/veggieseasons/lib/styles.dart Outdated Show resolved Hide resolved

LongCatIsLooong reviewed Nov 20, 2020

View reviewed changes

Removed unnecessary imports. Added AnnotatedRegions to search and lis…

d6307ad

…t screens.

LongCatIsLooong reviewed Nov 23, 2020

View reviewed changes

ADVavvas added 2 commits November 23, 2020 22:21

Changed AnnotatedRegion value to platformBrightness

9c564ad

Changed AnnotatedRegion value to platformBrightness (MediaQuery.platf…

70b661c

…ormBrightnessOf)

LongCatIsLooong approved these changes Nov 24, 2020

View reviewed changes

redbrogdon merged commit 7c5fecb into flutter:master Dec 2, 2020

Styling revamp using CupertinoThemeData and CupertinoTextThemeData #593

Styling revamp using CupertinoThemeData and CupertinoTextThemeData #593

Uh oh!

Conversation

ADVavvas commented Nov 12, 2020

Uh oh!

redbrogdon left a comment

Choose a reason for hiding this comment

Uh oh!

ADVavvas commented Nov 15, 2020

Uh oh!

redbrogdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ADVavvas commented Nov 19, 2020

Uh oh!

redbrogdon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Nov 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ADVavvas commented Nov 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

ADVavvas commented Nov 26, 2020

Uh oh!

LongCatIsLooong commented Dec 1, 2020

Uh oh!

redbrogdon commented Dec 2, 2020

Uh oh!

redbrogdon commented Dec 2, 2020

Uh oh!

Uh oh!

LongCatIsLooong commented Nov 20, 2020 •

edited

Loading