-
Notifications
You must be signed in to change notification settings - Fork 29.1k
Make CupertinoTabScaffold restorable #67770
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
Make CupertinoTabScaffold restorable #67770
Conversation
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.
LGTM, just two nits I think: a spelling error and an indentation.
@@ -289,43 +292,80 @@ class CupertinoTabScaffold extends StatefulWidget { | |||
/// Defaults to true and cannot be null. | |||
final bool resizeToAvoidBottomInset; | |||
|
|||
/// Restoration ID to save and restore the state of the [CupertinoTabScaffold]. | |||
/// | |||
/// This property only has an affect when no [controller] has been provided: |
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.
affect => effect
/// will persist and restore the currently selected tab index. If a | ||
/// [controller] has been provided, it is the responsibility of the owner of | ||
/// that controller to persist and restore it, e.g. by using a | ||
/// [RestorableCupertinoTabController]. |
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.
Are there a bunch of new restorable versions of classes now to support state restoration? This is the first state restoration PR I've taken a close look at.
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.
There are a few. Another example is the RestorableTextEditingController.
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.
Ah thanks, will study up.
} | ||
|
||
_internalController?.dispose(); |
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 think the indentation is off here if I'm seeing this right.
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.
Good eye!
if (_value != null) { | ||
// Scheduling a microtask for dispose to give other entities a chance | ||
// to remove their listeners first. | ||
scheduleMicrotask(_value!.dispose); |
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 had never heard of scheduleMicrotask before, good to know.
expect(find.text('Content 3'), findsNothing); | ||
}); | ||
|
||
testWidgets('switch from internal to external controller with state restoration', (WidgetTester tester) async { |
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.
Thanks for testing the controller swap too! That logic was a bit complicated.
Description
CupertinoTabScaffold can now restore the currently selected tab index.
Related Issues
#62916
Tests
I added the following tests:
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
Did any tests fail when you ran them? Please read Handling breaking changes.