-
-
Notifications
You must be signed in to change notification settings - Fork 220
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.
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
Feature, solid background #83 #84
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this PR.
Left some comments that need to be addressed before lading this.
}) : assert(items.length >= 2 && items.length <= 5), | ||
super(key: key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary changes
@@ -217,4 +223,7 @@ class BottomNavyBarItem { | |||
/// This will take effect only if [title] it a [Text] widget. | |||
final TextAlign? textAlign; | |||
|
|||
/// The [background] solid color defined when this item is selected. Defaults | |||
/// to [false]. | |||
final bool solidBackground; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change it to: If [true], use the solid color for the background of this item. Defaults to [false].
? item.solidBackground | ||
? Colors.white | ||
: item.activeColor.withOpacity(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, the property is called solidBackground
but it changes the color of the icon as well. You can either rename the property name to something like useSolidColors
or revert this change.
color: item.solidBackground | ||
? Colors.white | ||
: item.activeColor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Fixes #83