Skip to content

Conversation

@bleroux
Copy link
Contributor

@bleroux bleroux commented Nov 3, 2022

Description

This PR update ScaffoldMessengerState.showSnackBar to explicitly state that each Scaffold shows its own SnackBar instance.

(I looked up through SnackBar, Scaffold and ScaffoldMessenger documentation for a place to mention this. Mentioning this in ScaffoldMessengerState.showSnackBar seems to be the most relevant place as this method is directly referenced from SnackBar and ScaffolfMessenger documentation and because it is the public way to show a snack bar).

Related Issue

Fixes #105406

Tests

Documentation update only

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Nov 3, 2022
@bleroux bleroux requested a review from Piinks November 3, 2022 15:03
@bleroux bleroux force-pushed the fix_improve_showSnackbar_documentation branch from 2aefea5 to 1a71e45 Compare November 3, 2022 20:58
@Piinks Piinks added d: api docs Issues with https://api.flutter.dev/ documentation labels Nov 3, 2022
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to add this to the ScaffoldMessenger, Scaffold and SnackBar classes, in addition to showSnackBar.

Reading this on its own, I wonder, what are registered Scaffolds? Do we document that anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to add this to the ScaffoldMessenger, Scaffold and SnackBar classes, in addition to showSnackBar

I first wondered that too but reached the conclusion that showSnackBar is a central place for this comment:

  • Scaffold documentation has a lot of content and SnackBar is not mentioned there.
  • ScaffoldMessenger quickly emphasizes the showSnackBar method so it made sense
    to focus on showSnackBar documentation.

But revisiting this, I see several possible improvements:

  • As you mentioned, "registered Scaffolds" could be more explicit (this is explained in ScaffoldMessengerState documentation).
  • showMaterialBanner could be improved in a similar way.
  • SnackBar (and MaterialBanner) are not mentioned at all in Scaffold documentation. Adding a reference to them in the ‘See also’ section makes sense.
  • Stating that each scaffold has its own instance of SnackBar is not fully correct. There is only one instance of SnackBar (the one passed to showSnackBar). As it is a stateful widget and it is inserted into the tree in multiple locations, the framework will create a separate _SnackBarState instance for each scaffold. I’m not sure about how to explain this in a comprehensive way.

I updated the PR with those changes but the wording could probably be improved.

@bleroux bleroux force-pushed the fix_improve_showSnackbar_documentation branch from 1a71e45 to 671d51c Compare November 14, 2022 08:55
@bleroux bleroux requested a review from Piinks November 14, 2022 15:01
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! thanks again for always being so thorough!
LGTM-stamp

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 18, 2022
@auto-submit auto-submit bot merged commit 710e708 into flutter:master Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 18, 2022
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 18, 2022
* 70b7445 Reland Added Badge.count constructor (flutter/flutter#115566)

* 31f8631 fa7e1965e [Impeller] Fix glyph atlas uploads and renders (flutter/engine#37691) (flutter/flutter#115556)

* a1ea383 Label should always be aligned with text in filled input decoration (flutter/flutter#115540)

* c2b2950 Add selection feedback for both selection area and text field (flutter/flutter#115373)

* 0344407 Rev package:pub_semver to the latest version (flutter/flutter#115570)

* ac06523 Add Material 3 support for `Slider` - Part 2  (flutter/flutter#114624)

* b181d07 a2fa4e9 cirrus to luci (flutter/plugins#6711) (flutter/flutter#115573)

* e1efd0d b241e69fd [ui] reland add docs to FragmentShader (flutter/engine#37699) (flutter/flutter#115578)

* efb0694 Remove unused flutter_attach_test_fuchsia (flutter/flutter#115515)

* a5a368c 487ee66f6 [macOS] Merge FlutterRenderer and implementation (flutter/engine#37696) (flutter/flutter#115581)

* 4ff7fc6 Fixes a bug where dragging a collapsed handle in TextField does not vibrate (flutter/flutter#115586)

* 20be280 da9534ea6 [macOS] Consolidate external texture classes (flutter/engine#37703) (flutter/flutter#115585)

* 8a7102e Roll Flutter Engine from da9534ea6534 to d955a72c5604 (3 revisions) (flutter/flutter#115589)

* e1903a2 Roll Flutter Engine from d955a72c5604 to 1e1a4ab3c993 (4 revisions) (flutter/flutter#115592)

* 78390a0 Roll Flutter Engine from 1e1a4ab3c993 to b65c24ce621a (2 revisions) (flutter/flutter#115598)

* 75a0a72 [devicelab] measure entire release folder size, zipped (flutter/flutter#115597)

* 59a01b6 Roll Flutter Engine from b65c24ce621a to 49b52db603cc (3 revisions) (flutter/flutter#115606)

* ec03f1c Revert "[devicelab] measure entire release folder size, zipped (#115597)" (flutter/flutter#115609)

* 710e708 Improve showSnackBar documentation (flutter/flutter#114612)

* 915c3de Roll Flutter Engine from 49b52db603cc to 80b25a302b4c (2 revisions) (flutter/flutter#115608)

* 450f162 Roll Flutter Engine from 80b25a302b4c to e812122e4060 (2 revisions) (flutter/flutter#115614)

* 0b33b85 [devicelab] measure entire release folder size, zipped (flutter/flutter#115612)

* 9379c32 Revert "[devicelab] measure entire release folder size, zipped (#115612)" (flutter/flutter#115617)

* b746557 f27666d2f [macOS] Merge FlutterBackingStore implementations (flutter/engine#37730) (flutter/flutter#115616)

* 5487a7d Roll Flutter Engine from f27666d2f4da to 39f546585b0b (2 revisions) (flutter/flutter#115618)

* f261c2f update comments (flutter/flutter#115603)

* 9c9f781 04aea3c47 iOS PlatformView only sets a maskView when necessary (flutter/engine#37434) (flutter/flutter#115621)

* 6926960 4ca2c1d78 Roll Skia from 55f654bf5cff to 9d56e506b4df (13 revisions) (flutter/engine#37739) (flutter/flutter#115625)

* de4c0b1 Use `double.isNaN` instead of `... == double.nan` (which is always false) (flutter/flutter#115424)

* a655f85 a62736769 Roll Skia from 9d56e506b4df to d693b4b9fe5e (5 revisions) (flutter/engine#37741) (flutter/flutter#115640)

* 18c8727 f092cd826 Roll Fuchsia Mac SDK from SVtX810D2U_ZgBcpx... to tklUfTsSOVKk49tYq... (flutter/engine#37742) (flutter/flutter#115643)
@bleroux bleroux deleted the fix_improve_showSnackbar_documentation branch November 19, 2022 08:46
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App d: api docs Issues with https://api.flutter.dev/ f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve ScaffoldMessenger/Scaffold/Snackbar docs

3 participants