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

Remove CupertinoSystemColors in favor of CupertinoColors #40566

Merged
merged 9 commits into from Sep 25, 2019

Conversation

LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Sep 16, 2019

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

@fluttergithubbot fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Sep 16, 2019
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM

Just a very tiny spelling nitpick :)

darkHighContrastColor: Color.fromARGB(255, 174, 174, 178),
);

/// A second-level shade of grey.
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

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.

@LongCatIsLooong
Copy link
Contributor Author

@HansMuller @justinmc should I apply /gray/grey/ to existing constant names (e.g. inactiveGray, darkBrackgroundGray) too?

@LongCatIsLooong LongCatIsLooong added the c: API break Backwards-incompatible API changes label Sep 17, 2019
@@ -137,8 +577,6 @@ class CupertinoColors {
///
/// * [CupertinoUserInterfaceLevel], an [InheritedWidget] that may affect color
/// resolution of a `CupertinoDynamicColor`.
/// * [CupertinoSystemColors], an [InheritedWidget] that exposes system colors
/// of iOS 13+.
/// * https://developer.apple.com/documentation/uikit/uicolor/3238042-resolvedcolor.
@immutable
class CupertinoDynamicColor extends Color {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we pass 0 up to the constructor instead of just passing _effectiveColor.value and not overriding value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@LongCatIsLooong LongCatIsLooong merged commit deb4184 into flutter:master Sep 25, 2019
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: API break Backwards-incompatible API changes f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants