[State Restoration] Material Dialogs, Cupertino Dialogs, CupertinoTabScaffold/TabView, CupertinoTextFields #409
Conversation
| 'alert_with_title_press_demo_dialog_route'); | ||
| } | ||
|
|
||
| void setSelectedValue(String value) { |
There was a problem hiding this comment.
make private?
| void setSelectedValue(String value) { | |
| void _setSelectedValue(String value) { |
| } | ||
|
|
||
| void setSelectedValue(String value) { | ||
| setState(() { |
There was a problem hiding this comment.
In the old code, this only happened when value != null - should we keep that behavior?
There was a problem hiding this comment.
Yeah, sorry about that. I think I confused myself moving between NNBD and non-NNBD codebases, since the gallery is still pre-NNBD
| void _showDemoDialog({BuildContext context, Widget child}) { | ||
| showCupertinoDialog<String>( | ||
| static Route<String> _alertDemoDialog( | ||
| BuildContext context, Object arguments) { |
There was a problem hiding this comment.
nit (although I am not super familiar with the code formatting style guide here): I'd add a trailing comma behind arguments and format this in multiple lines.
| onPressed: () { | ||
| Navigator.of( | ||
| context, | ||
| rootNavigator: true, |
There was a problem hiding this comment.
Why rootNavigator: true here and elsewhere? Isn't the dialog just pushed into the closest enclosing dialog?
There was a problem hiding this comment.
Yeah, you're right. Again, I just copied over the behavior that was in the existing code, but it's not necessary so I removed it
lib/demos/material/dialog_demo.dart
Outdated
| final themes = InheritedTheme.capture( | ||
| from: context, | ||
| to: Navigator.of( | ||
| context, | ||
| rootNavigator: true, | ||
| ).context, | ||
| ); |
There was a problem hiding this comment.
Why do we need to capture these themes? My understanding is that is is capturing the themes between the Navigator that will own the route and the root Navigator. However, everything above the Navigator that will own the route should already be visible to the route, so there shouldn't be a need to capture?
There was a problem hiding this comment.
Same question for the instances below.
There was a problem hiding this comment.
I was just copying the default behavior without attempting to modify anything, but you're right, it's unnecessary. Removed it!
lib/demos/material/dialog_demo.dart
Outdated
| child: Theme( | ||
| data: Theme.of(context), |
There was a problem hiding this comment.
Why this? Isn't this just wrapping everything in the same ThemeData that children could access via Theme.of even if it wasn't re-wrapped like this?
There was a problem hiding this comment.
Same question for the instances below.
|
Addressed the code review and added action sheet state restoration, since that has made it into the |
|
Odd, the |
|
The web benchmark tests have been fixed with flutter/packages#281 and is unrelated to this change. |
|
The golden tests are failing, regardless of whether they have been updated or not. In any case, these changes should not have resulted in the golden failures seen since the state restoration changes do not contain any UI changes. |
|
Merging with the failed run goldens, since that doesn't seem related to the changes made in this PR |
Related to flutter/flutter#62916.
Made the following demos state restorable:
except ActionSheets/Modals, those will come when the framework changes ([State Restoration] CupertinoModalPopupRoute flutter#74805) have landed)