Skip to content

fix: bottom navigation bar colors #107924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
darrenaustin merged 2 commits into from
Sep 20, 2022
Merged

fix: bottom navigation bar colors #107924

darrenaustin merged 2 commits into from
Sep 20, 2022

Conversation

@talamaska
Copy link
Contributor

@talamaska talamaska commented Jul 19, 2022 •
edited
Loading

add: _effectiveIconTheme function
add: logic for setting the effective selected/unselected icon theme
change: use the new effective selected/unselected Icon theme when generating _BottomNavigationTile
change: create separate tweens for the icon and for the label
add: tests to capture the issues with the label style color, matching the docs
fix: BottomNavigationBarItem label's color is not overridden neither by selectedLabelStyle nor unselectedLabelStyle
add: legacy flag for preserving the old behavior

#64358
#107925
#27753

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jul 19, 2022
@talamaska talamaska mentioned this pull request Jul 19, 2022
Closed
8 tasks
@talamaska talamaska force-pushed the fix/64358-bottom-navigation-bar-colors branch from a459810 to 7ef27d6 Compare July 19, 2022 17:59
@darrenaustin darrenaustin self-requested a review July 22, 2022 19:59
darrenaustin
darrenaustin suggested changes Aug 30, 2022
View reviewed changes
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

@talamaska thanks for the contribution. Sorry for the lag on the review.

This mostly looks good. I am not a fan of the useLegacyColorScheme type flags, but I assume this is to work around a breaking change?

@talamaska
Copy link
Contributor Author

talamaska commented Aug 31, 2022 •
edited
Loading

@darrenaustin implemented the suggestions.

@talamaska
fix: bottom navigation bar colors
20efa67 
add: _effectiveIconTheme function
add: logic for setting the effective selected/unselected icon theme
change: color tweens to use the effective label style color if not null
change: use the new effective selected/unselected Icon theme when generating _BottomNavigationTile
add: tests to capture the issues with the label style color, matching the docs
fix: BottomNavigationBarItem title's color is not overriden neither by selectedLabelStyle nor unselectedLabelStyle
remove: trailing space


implement PR suggestions

add: more documentation for the useLegacyColorScheme flag
change: simplify effectiveIconTheme
@talamaska talamaska force-pushed the fix/64358-bottom-navigation-bar-colors branch from a65cd6c to 20efa67 Compare August 31, 2022 09:49
@talamaska
Merge remote-tracking branch 'origin/fix/64358-bottom-navigation-bar-…
e21b81f 
…colors' into fix/64358-bottom-navigation-bar-colors
@talamaska
Copy link
Contributor Author

talamaska commented Sep 15, 2022 •
edited
Loading

@darrenaustin @Hixie Am I going to wait for another 6 months for someone to finalize the review and merge this PR. The contributing experience so far is not the greatest one.

darrenaustin
darrenaustin approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@darrenaustin darrenaustin left a comment

Choose a reason for hiding this comment

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

LGTM.

@darrenaustin
Copy link
Contributor

darrenaustin commented Sep 15, 2022 •
edited
Loading

@darrenaustin @Hixie Am I going to wait for another 6 months for someone to finalize the review and merge this PR. The contributing experience so far is not the greatest one.

@talamaska my sincerest apologies. This is entirely my fault for letting this languish so long. I should have responded in a more timely manner. I will strive to do better in the future. We do appreciate your contribution. Thx.

@talamaska
Copy link
Contributor Author

talamaska commented Sep 15, 2022

@talamaska my sincerest apologies. This is entirely my fault for letting this languish so long. I should have responded in a more timely manner. I will strive to do better in the future. We do appreciate your contribution. Thx.

@darrenaustin thank you for responding. I apologize for my harsh comment. In my defense I really tried to be patient to this point

@talamaska
Copy link
Contributor Author

talamaska commented Sep 19, 2022

@darrenaustin when can I expect this merged?

@darrenaustin darrenaustin merged commit 888b141 into flutter:master Sep 20, 2022
@darrenaustin
Copy link
Contributor

darrenaustin commented Sep 20, 2022

@darrenaustin thank you for responding. I apologize for my harsh comment. In my defense I really tried to be patient to this point

Not at all, it was completely justified frustration. Again, my apologies.

@darrenaustin when can I expect this merged?

Done. Not sure why it didn't automerge after the review, I thought the label was set on it.

This was referenced Sep 20, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 20, 2022
@engine-flutter-autoroll
888b141 fix: bottom navigation bar colors (flutter/flutter#107924)
7019a2c
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Sep 20, 2022
@engine-flutter-autoroll
888b141 fix: bottom navigation bar colors (flutter/flutter#107924)
0c56e2e
This was referenced Oct 3, 2022
Closed
Closed
@TahaTesser TahaTesser mentioned this pull request Oct 27, 2022
Closed
@talamaska
Copy link
Contributor Author

talamaska commented Jan 5, 2023

@darrenaustin any idea when this can end up in stable? I don't see it in Beta as well. It is still just in master

@rscircus
Copy link

rscircus commented Feb 25, 2023

Hm, just ran into this, too. It's a rather old bug. Interesting.

@talamaska
Copy link
Contributor Author

talamaska commented Feb 27, 2023

Hm, just ran into this, too. It's a rather old bug. Interesting.

It was released in stable. Could you please try setting useLegacyColorScheme to false. The issue should be resolved.

@rscircus
Copy link

rscircus commented Feb 27, 2023

Moved to SnakeNavigationBar meanwhile...

@sejun2
Copy link

sejun2 commented Jul 20, 2023

https://api.flutter.dev/flutter/material/BottomNavigationBar/useLegacyColorScheme.html
This explains that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@darrenaustin darrenaustin darrenaustin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

@talamaska @darrenaustin @rscircus @sejun2