Repository navigation
Remove CupertinoSystemColors in favor of CupertinoColors - #40566
Conversation
fluttergithubbot
added
p: cupertino_ui
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM
Just a very tiny spelling nitpick :)
| darkHighContrastColor: Color.fromARGB(255, 174, 174, 178), | ||
| ); | ||
|
|
||
| /// A second-level shade of grey. |
There was a problem hiding this comment.
Some of your comments spell it "grey" and some spell it "gray". I guess everything should be "gray" since that's how Apple spells it?
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
LGTM too.
In material/colors.dart will spell "grey" which I think is the British version. For the sake of consistency, we should probably carry on with grey.
|
@HansMuller @justinmc should I apply |
| /// of iOS 13+. | ||
| /// * https://developer.apple.com/documentation/uikit/uicolor/3238042-resolvedcolor. | ||
| @immutable | ||
| class CupertinoDynamicColor extends Color { |
There was a problem hiding this comment.
why do we pass 0 up to the constructor instead of just passing _effectiveColor.value and not overriding value?
There was a problem hiding this comment.
Then the constructor can't be made const (#39430). IMO the benefit of being able to make a const CupertinoDynamicColor and use it in a function as the default parameter outweighs the cost, e.g.
const Color _kDefaultColor = Color(...);
class ThumbPainter extends CustomPainter {
const ThumbPainter({
this.color = _kDefaultColor,
}) : super(...);
}Now if I want to make _kDefaultColor a CupertinoDynamicColor, without the const constructor I'll have to break the API. Also works better with NNBD.
6 participants
Description
Making iOS 13 colors compile-time constants. Since we don't change the appearance of widgets based on the OS version, dynamically updating color values may not be the best strategy. This would allow us to use these dynamic colors as default parameters.
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?