Added bottom navigation. #5877
Added bottom navigation. #5877
Conversation
There was a problem hiding this comment.
typedef docs should always start with the word "Signature".
There was a problem hiding this comment.
"is useful in order" -> "can be used"
There was a problem hiding this comment.
Fixed anyway. Haha.
There was a problem hiding this comment.
i don't know what this sentence means
There was a problem hiding this comment.
newline between identifier and docs for next identifier
There was a problem hiding this comment.
newline after "action".
Describe the manual styling that is needed.
There was a problem hiding this comment.
I think "radially growing" means "growing with spokes from a point" which doesn't seem right here? I'm not sure.
I'd go with something like "the color of the iris animation" or some such.
There was a problem hiding this comment.
Also, that last sentence should probably be "Only has an effect when the [BottomNavigation.type] is [BottomNavigationType.shifting]."
In general I've noticed that you use the word "case" probably more than warranted. I recommend trying to avoid it. :-)
There was a problem hiding this comment.
This probably has to do with my trying to keep documentation nice and dry. Still perfecting my technique. 😃
There was a problem hiding this comment.
as a second paragraph, include a detailed description of what it is.
as a third paragraph, a description of how to use it.
There was a problem hiding this comment.
currentIndex should be private
There was a problem hiding this comment.
"jank" usually means "lost frames". I think you mean something else here, maybe "jiggling"?
There was a problem hiding this comment.
Called is 'instability'. The problem was actually making the animation janky.
There was a problem hiding this comment.
we always (not "we're always")
There was a problem hiding this comment.
Seems like a did some sentence refactoring and messed up the tense.
There was a problem hiding this comment.
blank line here is weird as it breaks the if statement
There was a problem hiding this comment.
i'm surprised the analyzer didn't complain; this should be "animating.isNotEmpty" (which will be quicker, since it'll abort at the first matching animation instead of counting all of them)
There was a problem hiding this comment.
why is there a material here? Materials are pretty heavy-weight, but you don't seem to actually use this one for anything other than a background color and shadow? Why not put it outside the stack? I don't really follow.
There was a problem hiding this comment.
Needed for the shadow.
There was a problem hiding this comment.
this is the same elevation as the other material, which doesn't seem right.
There was a problem hiding this comment.
Needed for the splashes. Removed elevation.
There was a problem hiding this comment.
are you sure this is the right order? why was it above the bottom sheets before but not now?
There was a problem hiding this comment.
please add a test that verifies that we're getting the right z-order when you have all these widgets present
There was a problem hiding this comment.
Well, according to the spec, snack bars should be below bottom navigation and bottom sheets should be above bottom navigation. There wasn't any rule for snack bars and bottom sheets per se, but you can infer the order now that bottom navigation is in the equation.
There was a problem hiding this comment.
Call this kBottomNavigationBarHeight for consistency. It should go in constants.dart.
There was a problem hiding this comment.
We haven't been terribly consistent about formatting the doc for callbacks, but in the interest of improving things ... this one should start with a line like:
/// The signature of the [BottomNavigationBar.onAction] callback.
There was a problem hiding this comment.
Avoid reiterating a type name as its definition. In this case it would be enough to say: Defines the layout and behavior of a [BottomNavigationBar].
There was a problem hiding this comment.
Same comment as above, Explain how a shifting BottomNavigationBar lays out, what you mean by "shifting", etc.
There was a problem hiding this comment.
Refer to [BottomNavigationBar.actions] here.
There was a problem hiding this comment.
This should be a const constructor
There was a problem hiding this comment.
We decided to leave like this in order to keep the asserts.
There was a problem hiding this comment.
Calling this an "action" doesn't sound right, since it's just a set of visual elements, not a behavior. You could call it a BottomNavigationBarLabel, and rename its label field "title".
There was a problem hiding this comment.
Called it DestinationLabel.
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
Just getting the naming/doc feedback out of the way.
There was a problem hiding this comment.
Not clear what the "Needs manual styling..." here and below means. Provide an explanation.
There was a problem hiding this comment.
No need to mention "radially growing". But you should explain how the color is used. For example:
If the navigation bar's type is [BottomNavigationBarType.shifting] then the entire bar is flooded with the backgroundColor when this label is tapped.
There was a problem hiding this comment.
As before, don't just reiterate the class name. In this case you could say:
A row of labels typically used to select among a small number of views.
There was a problem hiding this comment.
I added an explanation like @Hixie suggested. I'll push in a bit.
There was a problem hiding this comment.
Pushed fix. PTAL.
There was a problem hiding this comment.
This class, and all of the related types (sorry) should be called BottomNavigationBar. The spec begins with "Bottom navigation bars make it easy ..." All of the similar material design element Flutter classes are called FooBar :-).
There was a problem hiding this comment.
This should be called onTap().
There was a problem hiding this comment.
Replace this with the description of what BottomNavigationType means from above.
There was a problem hiding this comment.
It would be a bit more apt to call this _rebuild()
There was a problem hiding this comment.
action => DestinationLabel
There was a problem hiding this comment.
action => DestinationLabel
There was a problem hiding this comment.
The BottomNavigationBar's DestinationLabels have fixed equal widths.
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
You should take a another pass through the dartdoc to sync up with the new names.
There was a problem hiding this comment.
The location and size of the BottomNavigationBar DestinationLabels animate larger when then are tapped.
There was a problem hiding this comment.
ineractive [sic]
I think a better way to describe this is: A material widget displayed at the bottom of an app for selecting among a small number of views.
There was a problem hiding this comment.
A bottom navigation bar is ...
There was a problem hiding this comment.
layed => laid
You could just say that the BottomSheet is stacked on top of the BottomNavigationBar.
There was a problem hiding this comment.
BottomNavigation => BottomNavigationBar
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
Most of the feedback here is wordsmithing. This PR appears to be pretty close to its debut.
There was a problem hiding this comment.
This would be the place to explain how the bottom navigation bar is laid out vis the scaffold's bottom sheet and its snackbar.
There was a problem hiding this comment.
You need to explain that it's the responsibility of the callback's owner to keep track of the current index and to call setState() to rebuild its bottom bar.
|
This looks ready to go, you just need to add an explanation of how the widget that creates the bottom bar needs to keep track of its currentIndex and rebuild when the onTap() callback fires. LGTM. Victory! |
dbd2f57 Roll src/third_party/skia 897f256413c1..f27b479f957d (12 commits) (flutter#5878) 76e9f79 Only check trailing spaces for dart and don't check for CRLF (flutter#5877) 9f8285a Remove all dependencies on Garnet. (flutter#5869) 503ba7c Dart SDK roll for 2018/07/26 (flutter#5876) 2c6fa0d Roll src/third_party/skia b64db38b854b..897f256413c1 (4 commits) (flutter#5875) f480e32 Support hot reload in corejit mode (flutter#5866) 66f87f4 Roll src/third_party/skia f3ac64df1741..b64db38b854b (1 commits) (flutter#5874) 2193ff4 Roll src/third_party/skia e54cd95dc3ae..f3ac64df1741 (1 commits) (flutter#5873) aef291b Unify trailing spaces / new lines (flutter#5871) 6193743 Roll src/third_party/skia 97613523e357..e54cd95dc3ae (1 commits) (flutter#5872) 7624c8a Roll src/third_party/skia 68300c270916..97613523e357 (2 commits) (flutter#5870) cfcb701 Roll src/third_party/skia 1c94a8fabed7..68300c270916 (4 commits) (flutter#5867) 1d1c8c7 Rename clip to clipBehavior in compositing.dart (flutter#5868) d559afb Support customizing standard accessibility actions on Android. (flutter#5823) 228cecc Make //flutter/synchronization Garnet free. (flutter#5865) 336c23f Remove //flutter/glue and use FML directly. (flutter#5862)
896441a Roll buildroot to 4fd7a27 (flutter#5880) de206ea Update GetDirectoryName namespace to the one in FML on Linux. (flutter#5879) dbd2f57 Roll src/third_party/skia 897f256413c1..f27b479f957d (12 commits) (flutter#5878) 76e9f79 Only check trailing spaces for dart and don't check for CRLF (flutter#5877) 9f8285a Remove all dependencies on Garnet. (flutter#5869) 503ba7c Dart SDK roll for 2018/07/26 (flutter#5876) 2c6fa0d Roll src/third_party/skia b64db38b854b..897f256413c1 (4 commits) (flutter#5875) f480e32 Support hot reload in corejit mode (flutter#5866) 66f87f4 Roll src/third_party/skia f3ac64df1741..b64db38b854b (1 commits) (flutter#5874) 2193ff4 Roll src/third_party/skia e54cd95dc3ae..f3ac64df1741 (1 commits) (flutter#5873) aef291b Unify trailing spaces / new lines (flutter#5871) 6193743 Roll src/third_party/skia 97613523e357..e54cd95dc3ae (1 commits) (flutter#5872) 7624c8a Roll src/third_party/skia 68300c270916..97613523e357 (2 commits) (flutter#5870) cfcb701 Roll src/third_party/skia 1c94a8fabed7..68300c270916 (4 commits) (flutter#5867) 1d1c8c7 Rename clip to clipBehavior in compositing.dart (flutter#5868) d559afb Support customizing standard accessibility actions on Android. (flutter#5823) 228cecc Make //flutter/synchronization Garnet free. (flutter#5865) 336c23f Remove //flutter/glue and use FML directly. (flutter#5862)
flutter/engine@95e91e3...8b8c52e git log 95e91e3..8b8c52e --date=short --no-merges --format='%%ad %%ae %%s' 2018-07-27 37626415&flutter#43;[email protected] Roll src/third_party/skia b935cf8e12d7..55b1e6168454 (1 commits) (flutter#5886) 2018-07-26 [email protected] [scenic] Expose compositor context from rasterizer. (flutter#5739) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia 5f1dc76d0cec..b935cf8e12d7 (1 commits) (flutter#5885) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia f27b479f957d..5f1dc76d0cec (1 commits) (flutter#5884) 2018-07-26 [email protected] Fix compilation trace memory management problem 2018-07-26 [email protected] Revert &flutter#34;Dart SDK roll for 2018/07/26 (flutter#5876)&flutter#34; (flutter#5881) 2018-07-26 [email protected] Roll buildroot to 4fd7a27 (flutter#5880) 2018-07-26 [email protected] Update GetDirectoryName namespace to the one in FML on Linux. (flutter#5879) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia 897f256413c1..f27b479f957d (12 commits) (flutter#5878) 2018-07-26 [email protected] Only check trailing spaces for dart and don&flutter#39;t check for CRLF (flutter#5877) 2018-07-26 [email protected] Remove all dependencies on Garnet. (flutter#5869) 2018-07-26 [email protected] Dart SDK roll for 2018/07/26 (flutter#5876) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia b64db38b854b..897f256413c1 (4 commits) (flutter#5875) 2018-07-26 [email protected] Support hot reload in corejit mode (flutter#5866) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia f3ac64df1741..b64db38b854b (1 commits) (flutter#5874) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia e54cd95dc3ae..f3ac64df1741 (1 commits) (flutter#5873) 2018-07-26 [email protected] Unify trailing spaces / new lines (flutter#5871) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia 97613523e357..e54cd95dc3ae (1 commits) (flutter#5872) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 68300c270916..97613523e357 (2 commits) (flutter#5870) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 1c94a8fabed7..68300c270916 (4 commits) (flutter#5867) 2018-07-25 [email protected] Rename clip to clipBehavior in compositing.dart (flutter#5868) 2018-07-25 [email protected] Support customizing standard accessibility actions on Android. (flutter#5823) 2018-07-25 [email protected] Make //flutter/synchronization Garnet free. (flutter#5865) 2018-07-25 [email protected] Remove //flutter/glue and use FML directly. (flutter#5862) 2018-07-25 [email protected] Package Skia licenses into a separate file from other third party libraries (flutter#5861) 2018-07-25 [email protected] Pass touch events to embedded Android views with dispatchTouchEvents. 2018-07-23 [email protected] Synthesize Android MotionEvents with the long form obtain method. 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 396661825f45..1c94a8fabed7 (8 commits) (flutter#5859) 2018-07-25 [email protected] Rename clip mode to clip behavior (flutter#5853) 2018-07-25 [email protected] Make the virtual display&flutter#39;s presentation window non focusable. (flutter#5845) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia c062b6be5a01..396661825f45 (3 commits) (flutter#5857) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 15020ea2cf44..c062b6be5a01 (1 commits) (flutter#5856) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 4c7a750e5543..15020ea2cf44 (1 commits) (flutter#5855) 2018-07-24 37626415&flutter#43;[email protected] Roll src/third_party/skia 75e5406be66a..4c7a750e5543 (2 commits) (flutter#5854) 2018-07-24 37626415&flutter#43;[email protected] Roll src/third_party/skia b8eeb808d84f..75e5406be66a (17 commits) (flutter#5852) 2018-07-24 [email protected] Remove check_roll from Travis (flutter#5846) The AutoRoll server is located here: http://localhost:8000 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC&flutter#39;d on the roll, and stop the roller if necessary.
flutter/engine@95e91e3...8b8c52e git log 95e91e3..8b8c52e --date=short --no-merges --format='%%ad %%ae %%s' 2018-07-27 37626415&flutter#43;[email protected] Roll src/third_party/skia b935cf8e12d7..55b1e6168454 (1 commits) (flutter#5886) 2018-07-26 [email protected] [scenic] Expose compositor context from rasterizer. (flutter#5739) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia 5f1dc76d0cec..b935cf8e12d7 (1 commits) (flutter#5885) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia f27b479f957d..5f1dc76d0cec (1 commits) (flutter#5884) 2018-07-26 [email protected] Fix compilation trace memory management problem 2018-07-26 [email protected] Revert &flutter#34;Dart SDK roll for 2018/07/26 (flutter#5876)&flutter#34; (flutter#5881) 2018-07-26 [email protected] Roll buildroot to 4fd7a27 (flutter#5880) 2018-07-26 [email protected] Update GetDirectoryName namespace to the one in FML on Linux. (flutter#5879) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia 897f256413c1..f27b479f957d (12 commits) (flutter#5878) 2018-07-26 [email protected] Only check trailing spaces for dart and don&flutter#39;t check for CRLF (flutter#5877) 2018-07-26 [email protected] Remove all dependencies on Garnet. (flutter#5869) 2018-07-26 [email protected] Dart SDK roll for 2018/07/26 (flutter#5876) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia b64db38b854b..897f256413c1 (4 commits) (flutter#5875) 2018-07-26 [email protected] Support hot reload in corejit mode (flutter#5866) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia f3ac64df1741..b64db38b854b (1 commits) (flutter#5874) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia e54cd95dc3ae..f3ac64df1741 (1 commits) (flutter#5873) 2018-07-26 [email protected] Unify trailing spaces / new lines (flutter#5871) 2018-07-26 37626415&flutter#43;[email protected] Roll src/third_party/skia 97613523e357..e54cd95dc3ae (1 commits) (flutter#5872) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 68300c270916..97613523e357 (2 commits) (flutter#5870) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 1c94a8fabed7..68300c270916 (4 commits) (flutter#5867) 2018-07-25 [email protected] Rename clip to clipBehavior in compositing.dart (flutter#5868) 2018-07-25 [email protected] Support customizing standard accessibility actions on Android. (flutter#5823) 2018-07-25 [email protected] Make //flutter/synchronization Garnet free. (flutter#5865) 2018-07-25 [email protected] Remove //flutter/glue and use FML directly. (flutter#5862) 2018-07-25 [email protected] Package Skia licenses into a separate file from other third party libraries (flutter#5861) 2018-07-25 [email protected] Pass touch events to embedded Android views with dispatchTouchEvents. 2018-07-23 [email protected] Synthesize Android MotionEvents with the long form obtain method. 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 396661825f45..1c94a8fabed7 (8 commits) (flutter#5859) 2018-07-25 [email protected] Rename clip mode to clip behavior (flutter#5853) 2018-07-25 [email protected] Make the virtual display&flutter#39;s presentation window non focusable. (flutter#5845) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia c062b6be5a01..396661825f45 (3 commits) (flutter#5857) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 15020ea2cf44..c062b6be5a01 (1 commits) (flutter#5856) 2018-07-25 37626415&flutter#43;[email protected] Roll src/third_party/skia 4c7a750e5543..15020ea2cf44 (1 commits) (flutter#5855) 2018-07-24 37626415&flutter#43;[email protected] Roll src/third_party/skia 75e5406be66a..4c7a750e5543 (2 commits) (flutter#5854) 2018-07-24 37626415&flutter#43;[email protected] Roll src/third_party/skia b8eeb808d84f..75e5406be66a (17 commits) (flutter#5852) 2018-07-24 [email protected] Remove check_roll from Travis (flutter#5846) The AutoRoll server is located here: http://localhost:8000 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff, who should be CC&flutter#39;d on the roll, and stop the roller if necessary.
3 participants
No description provided.