Conversation
Piinks
added
c: new feature
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
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.
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. :) |
| /// tooltip: 'Show Snackbar', | ||
| /// onPressed: () { | ||
| /// scaffoldKey.currentState.showSnackBar(snackBar); | ||
| /// ScaffoldMessenger.of(context).showSnackBar(snackBar); |
There was a problem hiding this comment.
Can't the of call return null if there's no messenger in the tree?
There was a problem hiding this comment.
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.
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.
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.
I've applied your suggestions, PTAL. :)
|
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.
|
Co-authored-by: Michael Goderbauer <[email protected]>
6 participants
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.