Skip to content

Add key to BottomNavigationBarItem#139617

Merged
auto-submit[bot] merged 5 commits intoflutter:masterfrom
Gibbo97:add-keys-to-bottom-navigation-bar-item
Jan 3, 2024
Merged

Add key to BottomNavigationBarItem#139617
auto-submit[bot] merged 5 commits intoflutter:masterfrom
Gibbo97:add-keys-to-bottom-navigation-bar-item

Conversation

@Gibbo97
Copy link
Contributor

@Gibbo97 Gibbo97 commented Dec 6, 2023

Pass key into _BottomNavigationTile

Adding a optional key parameter to BottomNavigationBarItem to be passed through to _BottomNavigationTile.
This will allow easier testing in some scenarios, and fix the splash appearing on the wrong tile.

#139615
#34833

@google-cla
Copy link

google-cla bot commented Dec 6, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Dec 6, 2023
@Gibbo97 Gibbo97 changed the title Add key BottomNavigationBarItem Add key to BottomNavigationBarItem Dec 6, 2023
@Gibbo97 Gibbo97 mentioned this pull request Dec 6, 2023
Closed
2 tasks
gspencergoog
gspencergoog reviewed Dec 8, 2023
View reviewed changes
packages/flutter/lib/src/widgets/bottom_navigation_bar_item.dart Outdated
Copy link
Contributor

@gspencergoog gspencergoog Dec 8, 2023

Choose a reason for hiding this comment

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

Instead of referencing a private API here, can you explain what the key is used for, and an example use case (doesn't have to be a code example, just a use case, although I wouldn't turn a code example down. :-) ).

Copy link
Contributor Author

@Gibbo97 Gibbo97 Dec 11, 2023

Choose a reason for hiding this comment

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

thanks for the review, have updated. Hope this is what you were after :)

@bleroux
Copy link
Contributor

bleroux commented Dec 11, 2023 •
edited
Loading

About the 'Linux analyze' step failure, the related error message that will help you getting this step green is :

Analyzing flutter...                                            

   info • Local variables should be final • packages/flutter/test/material/bottom_navigation_bar_test.dart:3129:5 • prefer_final_locals

@Gibbo97 Gibbo97 force-pushed the add-keys-to-bottom-navigation-bar-item branch from 3bb6ac3 to e27cc42 Compare December 13, 2023 04:05
@github-actions github-actions bot added a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. engine flutter/engine related. See also e: labels. f: routes Navigator, Router, and related APIs. c: tech-debt Technical debt, code quality, testing, etc. labels Dec 13, 2023
@Gibbo97 Gibbo97 force-pushed the add-keys-to-bottom-navigation-bar-item branch from e27cc42 to 6749436 Compare December 13, 2023 06:04
@github-actions github-actions bot removed a: tests "flutter test", flutter_test, or one of our tests tool Affects the "flutter" command-line tool. See also t: labels. engine flutter/engine related. See also e: labels. f: routes Navigator, Router, and related APIs. c: tech-debt Technical debt, code quality, testing, etc. labels Dec 13, 2023
@christopherfujino christopherfujino removed their request for review December 13, 2023 20:47
gspencergoog
gspencergoog approved these changes Jan 2, 2024
View reviewed changes
Copy link
Contributor

@gspencergoog gspencergoog left a comment

Choose a reason for hiding this comment

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

32384589-a60f0e74-c078-11e7-9bc1-e5b5287aea9d

(approved, modulo the comments below)

@gspencergoog
Copy link
Contributor

gspencergoog commented Jan 2, 2024

@Gibbo97 are you able to resolve the conflicts here and bring the PR up to date?

@Gibbo97 Gibbo97 force-pushed the add-keys-to-bottom-navigation-bar-item branch 3 times, most recently from 721e83d to c3d5fa2 Compare January 3, 2024 00:06
@Gibbo97 @gspencergoog
Improve documentation
e4b5c63 
using suggestions from code review

Co-authored-by: Greg Spencer <[email protected]>
@Gibbo97 Gibbo97 force-pushed the add-keys-to-bottom-navigation-bar-item branch from c3d5fa2 to e4b5c63 Compare January 3, 2024 00:08
@Gibbo97
Copy link
Contributor Author

Gibbo97 commented Jan 3, 2024

@gspencergoog ready to be merged

@gspencergoog
Copy link
Contributor

gspencergoog commented Jan 3, 2024

Okay, great, we'll need a second reviewer (I'll find someone) and then we can merge.

QuncCccccc
QuncCccccc approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@QuncCccccc QuncCccccc left a comment

Choose a reason for hiding this comment

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

LGTM:) Thanks for the contribution!

@gspencergoog gspencergoog added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 3, 2024
@auto-submit auto-submit bot merged commit f4cb3d2 into flutter:master Jan 3, 2024
@Gibbo97 Gibbo97 deleted the add-keys-to-bottom-navigation-bar-item branch January 3, 2024 21:01
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 3, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 3, 2024
@engine-flutter-autoroll
f4cb3d2 Add key to BottomNavigationBarItem (flutter/flutter#139617)
6b96252
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 5, 2024
@engine-flutter-autoroll
f4cb3d2 Add key to BottomNavigationBarItem (flutter/flutter#139617)
db203d3
@HansMuller HansMuller mentioned this pull request Jan 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024
@engine-flutter-autoroll
f4cb3d2 Add key to BottomNavigationBarItem (flutter/flutter#139617)
d5a4c12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gspencergoog gspencergoog gspencergoog approved these changes

@QuncCccccc QuncCccccc QuncCccccc approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Gibbo97 @bleroux @gspencergoog @QuncCccccc