-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Adds CupertinoTheme #23759
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
Adds CupertinoTheme #23759
Conversation
|
cc @HansMuller this is what I have in mind so far. The InheritedModel stuff and the deferring to Material Theme parts still need work. |
208ef47 to
6cda682
Compare
5759f3b to
199567d
Compare
|
@HansMuller sorry, this thing is a bit long but it's ready for a look. Should be based on the same ideas as your PR. I think we're going to want something different than the _CupertinoThemeInheritedData thing but the rest should be mostly unaffected. |
HansMuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a little feedback based on reading some of the tests (as you suggested, I've started by reading the tests).
The tests all check the properties of themed components against constant values, like:
expect(decoration.color, CupertinoColors.activeOrange);
This kind of test could pass even if the component didn't consult the CupertinoTheme at all. Component tests that check a component's styles and colors have the expected default values seem worthwhile. Theme tests could could configure a theme with non-default values and verify that the components picked up those values:
expect(decoration.color, darkTestTheme.primaryColor);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of this file seems to consistently put the test name on the first line, the test closure on the next
9d40e11 to
97c2234
Compare
HansMuller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still reading through everything, just sending an update.
Most of the feedback is just doc nitpickery. And on that topic:
Although the words 'descendent' and 'descendant' are probably interchangeable, in the Flutter sources we use the latter almost exclusively.
d527597 to
c8fc81d
Compare
|
Since it seems to be getting closer, adding Matt as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there were a CupertinoThemeData.merge() method, roughly like TextStyle's merge then you could just say that the default MaterialBasedCupertinoThemeData will be merged with the cupertinoOverrideTheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried it might create more confusion on having to explain the difference between a CupertinoThemeData.merge, a CupertinoThemeData.copyWith, a MaterialBasedCupertinoThemeData.merge and a MaterialBasedCupertinoThemeData.copyWith.
examples/flutter_gallery/lib/demo/cupertino/cupertino_alert_demo.dart
Outdated
Show resolved
Hide resolved
examples/flutter_gallery/lib/demo/cupertino/cupertino_alert_demo.dart
Outdated
Show resolved
Hide resolved
c8fc81d to
8b53dd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: having a subclass here makes CupertinoThemeData less efficient to access (every access now has to have a potential dynamic dispatch). If we could just create a regular CupertinoThemeData directly with the fallback logic in the caller, rather than having a subclass, it might be better for performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I was debating about inheritance too. The compromise is basically using inheritance or interface (and composition here) but the base CupertinoThemeData can't be const constructed.
I don't have a strong opinion either way. Do you feel strongly one way? Material ThemeData is factory constructed with all the fields initializing logic but it sounds like Hans is moving away from that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we actually need this? we only need it in ThemeData because the default constructor does all kinds of magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just needed 2 different constructors for optional arguments and required arguments. But you point below about the raw constructor and method overlap is a good point. I'll rename this (since it's @protected and less important).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this explanation really doesn't help anyone who doesn't understand the subtleties of the class hierarchy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added more text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's very confusing that CupertinoThemeData.raw() doesn't return the same thing as CupertinoThemeData().raw()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the non-constructor method to noDefault() instead since it's more explicit
d0850d7 to
6416084
Compare
ac9017c to
a64290c
Compare
Fixes #18037
using InheritedModelMaterialBasedCupertinoThemeData is also based on InheritedModels where dependency is only created on individual properties readUpgrade Flow
Starting this PR, Cupertino widgets under a Material Theme will somewhat adapt to that Material Theme (such as the primaryColor, brightness etc).
To avoid Material adaptations and to let Cupertino widgets use a default blank CupertinoTheme, insert a
CupertinoThemeunder the MaterialThemesuch as viaor
in case the Material Theme comes from the MaterialApp.