Repository navigation
Rename DestinationLabel on BottomNavigationBar - #7281
Conversation
| } | ||
|
|
||
| /// An interactive destination label within [BottomNavigationBar] with an icon | ||
| /// An interactive item within [BottomNavigationBar] with an icon |
There was a problem hiding this comment.
I think I'd leave this as "destination label" so it matches the spec. In general we prefer the docs to use terms that don't match the class names since matching the class names doesn't help anyone. :-)
| /// labels laid out on top of a piece of material. It provies quick navigation | ||
| /// between top-level views of an app and is typically used on mobile. For | ||
| /// larger screens, side navigation may be a better fit. | ||
| /// The bottom navigation bar consists of multiple item in the form of |
| /// between top-level views of an app and is typically used on mobile. For | ||
| /// larger screens, side navigation may be a better fit. | ||
| /// The bottom navigation bar consists of multiple item in the form of | ||
| /// labels and/or icons laid out on top of a piece of material. It provides |
There was a problem hiding this comment.
prefer "labels, icons, or both"
| /// The bottom navigation bar consists of multiple item in the form of | ||
| /// labels and/or icons laid out on top of a piece of material. It provides | ||
| /// quick navigation between top-level views of an app and is typically used | ||
| /// on mobile. For larger screens, side navigation may be a better fit. |
|
I adjusted the comments based on your feedback. I removed the "primarily used on mobile" as it seems to have been pulled from the material spec directly and its the only real use case right now. I left in a note about larger format devices/screens (it's also in the spec) as it could be useful design advice. Let me know if you want that pulled. |
| /// larger screens, side navigation may be a better fit. | ||
| /// The bottom navigation bar consists of multiple items in the form of | ||
| /// labels, icons, or both laid out on top of a piece of material. It provides | ||
| /// quick navigation between the top-level views of an app. Note: for larger screens, |
There was a problem hiding this comment.
Please avoid the word "Note".
See https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-empty-prose
| /// between top-level views of an app and is typically used on mobile. For | ||
| /// larger screens, side navigation may be a better fit. | ||
| /// The bottom navigation bar consists of multiple items in the form of | ||
| /// labels, icons, or both laid out on top of a piece of material. It provides |
There was a problem hiding this comment.
grammar nit: i'd put a comma after "both" to make it clear that the next part doesn't only apply to the "both" option.
|
|
|
I'll be doing another PR later for a few other things so I can incorporate the feedback at that time. |
|
Yes for the changes, but it's fine if you do them in the coming days instead of right away. |
|
Done |
|
+1 thanks! |
|
@lukef would you like to add yourself to our AUTHORS file? https://github.com/flutter/flutter/blob/master/AUTHORS |
|
Added myself. Thanks! 👍 |
|
Merged. FYI @abarth for the API change. We should make sure we've not left any DestinationLabels anywhere. Luke, would you mind e-mailing flutter-dev to let them know about the API change? Thanks! |
|
And thanks for the contribution! Much appreciated! |
4 participants
DestinationLabeltoBottomNavigationBarItemBottomNavigationBarItem.labelstoBottomNavigationBarItem.items