-
-
Notifications
You must be signed in to change notification settings - Fork 216
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
added BottemNavigationBarTheme compatibility #72
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #72 +/- ##
==========================================
+ Coverage 79.74% 84.14% +4.39%
==========================================
Files 1 1
Lines 79 82 +3
==========================================
+ Hits 63 69 +6
+ Misses 16 13 -3
Continue to review full report at Codecov.
|
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 for this great contribution. Since this is a new feature could you please add tests to prevent this feature to break in the future?
Since this is a big breaking change it will take some time to to get it merged to prevent API breaks on users. I will get back to this on the new big release.
currently the |
I modified this package previously for personal use, but thought it might be a useful addition to the package.
This is a major change. It adds compatibility with
BottomNavigationBarTheme
andBottomNavigationBarThemeData
. This not only adds way more customisability but more importantly, allows for better integration with an apps theme throughThemeData
my implementation of individual per icon theming is a bit flaky (but works just fine), comments explain why.
I've updated the example to be the same but with current theming implementation. I updated the current tests so they pass. Some extra tests will probably have to be written but decided to poll for some opinions here first.