CupertinoActionSheet dark mode & fidelity#39215
CupertinoActionSheet dark mode & fidelity#39215LongCatIsLooong merged 12 commits intoflutter:masterfrom
Conversation
fluttergithubbot
added
f: cupertino
LongCatIsLooong
force-pushed
the
action-sheet+dynamic-color
branch
from
5e98bcd to
5bfa8e9
Compare
| @@ -35,25 +36,48 @@ const TextStyle _kActionSheetContentStyle = TextStyle( | |||
| // The white color doesn't paint any white, it's just the basis for the | |||
There was a problem hiding this comment.
This comment should be updated to black as well.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
This is a good example of how to use CupertinoDynamicColor. I guess you'll be opening a bunch of similar PRs like this as you convert the rest of the widgets?
| // Translucent, very light gray that is painted on top of the blurred backdrop | ||
| // as the action sheet's background color. | ||
| const Color _kBackgroundColor = Color(0xD1F8F8F8); | ||
| // These should be from System Materials which we don't have yet. They're eye-balled |
There was a problem hiding this comment.
Maybe write this comment as a TODO so it's easier to find later? Up to you though obviously!
| Color get dividerColor => _dividerPaint.color; | ||
| set dividerColor(Color value) { | ||
| if (value == _dividerPaint.color) | ||
| return; |
There was a problem hiding this comment.
Indent here (and maybe curly braces).
| await tester.tap(find.text('Go')); | ||
| await tester.pump(); | ||
|
|
||
| // Draw the overlay. |
There was a problem hiding this comment.
I think you meant to use a different comment here.
There was a problem hiding this comment.
Oh I meant to rewrite the test but completely forgot about it the next day. Thanks for catching this.
| // Translucent, very light gray that is painted on top of the blurred backdrop | ||
| // as the action sheet's background color. | ||
| const Color _kBackgroundColor = Color(0xD1F8F8F8); | ||
| // These should be from System Materials which we don't have yet. They're eye-balled |
There was a problem hiding this comment.
Should we be using systemBackground from CupertinoSystemColors instead of _kBackgroundColor? Likewise for the other eyeballed colors? The HIG docs say "Use system colors with system materials." Shouldn't we be using the system colors throughout this class?
There was a problem hiding this comment.
Ah sorry I just realized I got the value from the official sketch file. Unfortunately it doesn't seem to be one of the system colors.
@darrenaustin unfortunately there's an assert guarding that constructor. The analyzer complained when I tried removing the color parameter. |
| CupertinoSystemColors.fallbackValues.systemBlue.color.value, | ||
| ); | ||
|
|
||
| stateSetter(() { brightness = Brightness.dark; }); |
| // Draw the overlay using the light variant. | ||
| expect(find.byType(CupertinoActionSheet), paints..rect(color: const Color(0x66000000))); | ||
| expect( | ||
| tester.firstWidget<DefaultTextStyle>(find.widgetWithText(DefaultTextStyle, 'action')).style.color.value, |
There was a problem hiding this comment.
It's preferable to avoid .first and .firstWidget when that's possible. Could this lookup be factored into a function that returns action button's text style? Like:
TextStyle actionTextStyle() {
return tester.widget<DefaultTextStyle>(
find.descendant(
of: find.widgetWithText(CupertinoActionSheetAction, 'action'),
matching: find.byType(DefaultTextStyle),
)
).style;
}
3b3344a to
520a254
Compare
…ng/flutter into action-sheet+dynamic-color
6 participants
Description
Update some colors used by
CupertinoActionSheet.Related Issues
#35541
Tests
I added the following tests:
Screenshots
Light mode
Dark mode
Note that the text color is still in Orange since we haven't updated the default CupertinoThemeData.primaryColor.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?