-
Notifications
You must be signed in to change notification settings - Fork 27.2k
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
Re-land ScaffoldMessenger #66504
Re-land ScaffoldMessenger #66504
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.
I haven't looked at the tests yet.
Is there some way to avoid temporarily adding SnackBarAction,SnackBar.useScaffoldMessenger?
/// _counter++; | ||
/// }); | ||
/// if (_counter % 10 == 0) { | ||
/// _scaffoldMessengerKey.currentState.showSnackBar(const SnackBar( |
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.
In this case you do have access to the context, because State.context. If the incrementCounter method and its state were in a separate class, this "no context" scenario would be clearer.
I haven't been able to come up with another way yet. 🤔 Since SnackBarAction and SnackBar call on the SnackBar methods, and we are going to support both APIs temporarily, they have to know which one to call, Scaffold or ScaffoldMessenger. |
Marking as WIP, @HansMuller and I discussed a re-work that will simplify the way SnackBars can handle themselves in this 2-way API interim. |
@HansMuller applied the changes from our brainstorming session, PTAL. :) |
@@ -137,7 +137,7 @@ class _ToolbarContainerLayout extends SingleChildLayoutDelegate { | |||
/// icon: const Icon(Icons.add_alert), | |||
/// tooltip: 'Show Snackbar', | |||
/// onPressed: () { | |||
/// scaffoldKey.currentState.showSnackBar(snackBar); | |||
/// ScaffoldMessenger.of(context).showSnackBar(snackBar); |
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.
Can't the of
call return null if there's no messenger in the tree?
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 can, but in this case it there is a MaterialApp above in the sample code, so it has a ScaffoldMessenger by default.
/// | ||
/// * [debugCheckHasScaffoldMessenger], which asserts that the given context | ||
/// has a [ScaffoldMessenger] ancestor. | ||
static ScaffoldMessengerState of(BuildContext context) { |
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.
Should this include a call to debugCheckHasScaffoldMessenger
to assert with a nice message when it fails to find one it expected to find?
Also, a common pattern with these of
calls is to include a nullOk
optional bool
that allows it to not assert and just return null.
I realize we do also have this pattern in some places of just returning null, but I guess I'm asking which is more appropriate for the use case: defaulting to throwing if it isn't found, or defaulting to not throwing and making the developer check for it? I think I'd lean toward the former, because it'll make them think about their choice.
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.
At one point I had it the way you describe, but it was massively breaking at the time. It's been re-worked a fair bit since then, so I think I should be able to add this back now. Thank you!
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've applied your suggestions, PTAL. :)
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 pull request is not suitable for automatic merging in its current state.
|
This pull request is not suitable for automatic merging in its current state.
|
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.
Fly-by comment
Co-authored-by: Michael Goderbauer <goderbauer@google.com>
Description
This will re-introduce the ScaffoldMessenger, this time leaving in place the old API for SnackBars via the Scaffold. This will allow for an easier migration for our customers.
As such, this adds an assertion to ensure we are only using one SnackBar API, since they are not meant to be used interchangeably.
The documentation and samples have been updated to recommend ScaffoldMessenger in order to mitigate confusion during the interim.
Original PR: #64101
Migration Guide: flutter/website#4527
Follow-up work:
Related Issues
Fixes #62921
Tests
Oodles.
All existing SnackBar tests have been migrated and passed. Added a few more for new persistence across routes.
Several control tests for the original API have been preserved to ensure we are not breaking the old API while bringing in the new.
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.