Fix floating SnackBar throws when FAB is on the top #129274
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
f: material design
bleroux
changed the title
bleroux
force-pushed
the
show_floating_snackbar_at_bottom_when_FAB_on_top
branch
from
0b94627 to
4f99d30
Compare
Piinks
reviewed
Jun 23, 2023
Contributor
There was a problem hiding this comment.
This should use a switch statement. Dart 3 makes it really quite nice!
For example:
final BorderRadius childrenGroupBorderRadius = switch (type) {
CupertinoListSectionType.insetGrouped => _kDefaultInsetGroupedBorderRadius,
CupertinoListSectionType.base => BorderRadius.zero,
};
Contributor
Author
There was a problem hiding this comment.
Nice! I switched to Dart 3 switch 😄
bleroux
force-pushed
the
show_floating_snackbar_at_bottom_when_FAB_on_top
branch
3 times, most recently
from
ee17aa8 to
8a0fa25
Compare
Piinks
reviewed
Jul 10, 2023
Comment on lines
1140
to
1145
Contributor
There was a problem hiding this comment.
These can use || so => false does not need to be repeated.
Contributor
There was a problem hiding this comment.
Instead of a default case, can you enumerate the other values? That way if we ever add another case, we won't introduce a bug where it isn't accounted for. This is actually why we prefer to use switch statements with enums :)
Contributor
Author
There was a problem hiding this comment.
This is actually why we prefer to use switch statements with enums :)
I'm not used to rely on switch, thanks for your explanations 👍 .
In this particular case, it turned out that FloatingActionButtonLocation is not an enum (values are constants defined in an abstract class and they are initialized by instantiating various subclasses). I think we can't avoid adding a default case because this is not a real enum?
I pushed a commit where I enumerated all values (there are a lot ot them). It seems that this an edge case where maybe we can avoid listing all values for readability?
Contributor
There was a problem hiding this comment.
Oh wow I did not realize it was an enum! TIL something new! In that case (and sorry for not realizing it sooner) I would recommend throwing an error for the default case. That way, if we add a new location, there will be an error that let's us know we need to account for it here. :)
bleroux
force-pushed
the
show_floating_snackbar_at_bottom_when_FAB_on_top
branch
from
8a0fa25 to
162fafc
Compare
Piinks
reviewed
Jul 18, 2023
Contributor
There was a problem hiding this comment.
So this instead could be that default case that presents an error. :)
I know it is very verbose, but it will keep future bugs away.
bleroux
force-pushed
the
show_floating_snackbar_at_bottom_when_FAB_on_top
branch
from
162fafc to
6813883
Compare
bleroux
force-pushed
the
show_floating_snackbar_at_bottom_when_FAB_on_top
branch
from
6813883 to
0bc5fe4
Compare
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 25, 2023
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 25, 2023
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Jul 25, 2023
flutter/flutter@d7ed5dc...9def8f6 2023-07-25 [email protected] Proposal to add barrier configs for showDatePicker, showTimePicker and showAboutDialog. (flutter/flutter#130484) 2023-07-25 [email protected] Roll Flutter Engine from a7a842ee9ccd to 036c58f79307 (1 revision) (flutter/flutter#131244) 2023-07-25 [email protected] Roll Flutter Engine from 3baca2fe55c8 to a7a842ee9ccd (1 revision) (flutter/flutter#131243) 2023-07-25 [email protected] Roll Flutter Engine from 9a0192d965e0 to 3baca2fe55c8 (1 revision) (flutter/flutter#131241) 2023-07-25 [email protected] Roll Flutter Engine from ceb2674e82b4 to 9a0192d965e0 (3 revisions) (flutter/flutter#131230) 2023-07-25 [email protected] Roll Flutter Engine from 4fded78e5a01 to ceb2674e82b4 (2 revisions) (flutter/flutter#131229) 2023-07-25 [email protected] Roll Flutter Engine from ff02fa72acce to 4fded78e5a01 (2 revisions) (flutter/flutter#131225) 2023-07-24 [email protected] Roll Flutter Engine from a489c7496268 to ff02fa72acce (1 revision) (flutter/flutter#131221) 2023-07-24 [email protected] Roll Flutter Engine from 815b97157dc7 to a489c7496268 (3 revisions) (flutter/flutter#131218) 2023-07-24 [email protected] Roll Flutter Engine from 2b8d83fa20e3 to 815b97157dc7 (5 revisions) (flutter/flutter#131214) 2023-07-24 [email protected] Use toStringAsFixed in DecorationImage.toString (flutter/flutter#131026) 2023-07-24 [email protected] Roll Flutter Engine from aa876f6bec69 to 2b8d83fa20e3 (3 revisions) (flutter/flutter#131207) 2023-07-24 [email protected] Fix M3 TimePicker dial background uses incorrect color (flutter/flutter#131045) 2023-07-24 [email protected] Fix floating SnackBar throws when FAB is on the top (flutter/flutter#129274) 2023-07-24 [email protected] Update link to unbounded constraints error (flutter/flutter#131205) 2023-07-24 [email protected] Optimize SliverMainAxisGroup/SliverCrossAxisGroup paint function (flutter/flutter#129310) 2023-07-24 [email protected] [DropdownMenu] Close menu after editing is complete (flutter/flutter#130710) 2023-07-24 [email protected] Reduce usage of testUsingContext (flutter/flutter#131078) 2023-07-24 [email protected] Roll Packages from 2266a76 to 8028caf (13 revisions) (flutter/flutter#131196) 2023-07-24 [email protected] Fix material date picker behavior when changing year (flutter/flutter#130486) 2023-07-24 [email protected] Update Gallery demo app themes for Material3 compatibility (flutter/flutter#131093) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
XilaiZhang
added a commit
that referenced
this pull request
Jul 25, 2023
This reverts commit 97e0a05.
fluttermirroringbot
pushed a commit
that referenced
this pull request
Jul 28, 2023
## Description This PR is a reland of #129274 with a fix and new test related to the revert (#131303). It updates how a floating snack bar is positionned when a `Scaffold` defines a FAB with `Scaffold.floatingActionButtonLocation` sets to one of the top locations. **Before this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` can't be displayed and an assert throws in debug mode. **After this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` will be displayed at the bottom of the screen, above a `NavigationBar` for instance (the top FAB is ignored when computing the floating snack bar position).  ## Motivation This is a edge case related to a discrepancy between the Material spec and the Flutter `Scaffold` customizability: - Material spec states that a floating `SnackBar` should be displayed above a FAB. But, in Material spec, FABs are expected to be on the bottom. - Since #51465, Flutter `Scaffold` makes it valid to show a FAB on the top of the `Scaffold`. ## Related Issue fixes #128150 ## Tests Adds 2 tests.
LouiseHsu
pushed a commit
to LouiseHsu/flutter
that referenced
this pull request
Jul 31, 2023
## Description This PR updates how a floating snack bar is positionned when a `Scaffold` defines a FAB with `Scaffold.floatingActionButtonLocation` sets to one of the top locations. **Before this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` can't be displayed and an assert throws in debug mode. **After this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` will be displayed at the bottom of the screen, above a `NavigationBar` for instance (the top FAB is ignored when computing the floating snack bar position).  ## Motivation This is a edge case related to a discrepancy between the Material spec and the Flutter `Scaffold` customizability: - Material spec states that a floating `SnackBar` should be displayed above a FAB. But, in Material spec, FABs are expected to be on the bottom. - Since flutter#51465, Flutter `Scaffold` makes it valid to show a FAB on the top of the `Scaffold`. ## Related Issue fixes flutter#128150 ## Tests Adds 1 test.
LouiseHsu
pushed a commit
to LouiseHsu/flutter
that referenced
this pull request
Jul 31, 2023
…e top" (flutter#131303) Reverts flutter#129274 temporarily putting up a revert in case a fix is difficult context: [b/293202068](http://b/293202068) youtube integration tests failed
LouiseHsu
pushed a commit
to LouiseHsu/flutter
that referenced
this pull request
Jul 31, 2023
…#131475) ## Description This PR is a reland of flutter#129274 with a fix and new test related to the revert (flutter#131303). It updates how a floating snack bar is positionned when a `Scaffold` defines a FAB with `Scaffold.floatingActionButtonLocation` sets to one of the top locations. **Before this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` can't be displayed and an assert throws in debug mode. **After this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` will be displayed at the bottom of the screen, above a `NavigationBar` for instance (the top FAB is ignored when computing the floating snack bar position).  ## Motivation This is a edge case related to a discrepancy between the Material spec and the Flutter `Scaffold` customizability: - Material spec states that a floating `SnackBar` should be displayed above a FAB. But, in Material spec, FABs are expected to be on the bottom. - Since flutter#51465, Flutter `Scaffold` makes it valid to show a FAB on the top of the `Scaffold`. ## Related Issue fixes flutter#128150 ## Tests Adds 2 tests.
vashworth
pushed a commit
to vashworth/flutter
that referenced
this pull request
Aug 2, 2023
## Description This PR updates how a floating snack bar is positionned when a `Scaffold` defines a FAB with `Scaffold.floatingActionButtonLocation` sets to one of the top locations. **Before this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` can't be displayed and an assert throws in debug mode. **After this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` will be displayed at the bottom of the screen, above a `NavigationBar` for instance (the top FAB is ignored when computing the floating snack bar position).  ## Motivation This is a edge case related to a discrepancy between the Material spec and the Flutter `Scaffold` customizability: - Material spec states that a floating `SnackBar` should be displayed above a FAB. But, in Material spec, FABs are expected to be on the bottom. - Since flutter#51465, Flutter `Scaffold` makes it valid to show a FAB on the top of the `Scaffold`. ## Related Issue fixes flutter#128150 ## Tests Adds 1 test.
vashworth
pushed a commit
to vashworth/flutter
that referenced
this pull request
Aug 2, 2023
…e top" (flutter#131303) Reverts flutter#129274 temporarily putting up a revert in case a fix is difficult context: [b/293202068](http://b/293202068) youtube integration tests failed
vashworth
pushed a commit
to vashworth/flutter
that referenced
this pull request
Aug 2, 2023
…#131475) ## Description This PR is a reland of flutter#129274 with a fix and new test related to the revert (flutter#131303). It updates how a floating snack bar is positionned when a `Scaffold` defines a FAB with `Scaffold.floatingActionButtonLocation` sets to one of the top locations. **Before this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` can't be displayed and an assert throws in debug mode. **After this PR:** - When a FAB location is set to the top of the `Scaffold`, a floating `SnackBar` will be displayed at the bottom of the screen, above a `NavigationBar` for instance (the top FAB is ignored when computing the floating snack bar position).  ## Motivation This is a edge case related to a discrepancy between the Material spec and the Flutter `Scaffold` customizability: - Material spec states that a floating `SnackBar` should be displayed above a FAB. But, in Material spec, FABs are expected to be on the bottom. - Since flutter#51465, Flutter `Scaffold` makes it valid to show a FAB on the top of the `Scaffold`. ## Related Issue fixes flutter#128150 ## Tests Adds 2 tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR updates how a floating snack bar is positionned when a
Scaffolddefines a FAB withScaffold.floatingActionButtonLocationsets to one of the top locations.Before this PR:
Scaffold, a floatingSnackBarcan't be displayed and an assert throws in debug mode.After this PR:
Scaffold, a floatingSnackBarwill be displayed at the bottom of the screen, above aNavigationBarfor instance (the top FAB is ignored when computing the floating snack bar position).Motivation
This is a edge case related to a discrepancy between the Material spec and the Flutter
Scaffoldcustomizability:SnackBarshould be displayed above a FAB. But, in Material spec, FABs are expected to be on the bottom.Scaffoldmakes it valid to show a FAB on the top of theScaffold.Related Issue
fixes #128150
Tests
Adds 1 test.