Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

casvanluijtelaar
Copy link

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 and BottomNavigationBarThemeData. This not only adds way more customisability but more importantly, allows for better integration with an apps theme through ThemeData

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.

@codecov-io
Copy link

Codecov Report

Merging #72 (2f45ee8) into master (08d0b31) will increase coverage by 4.39%.
The diff coverage is 78.12%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
lib/bottom_navy_bar.dart 84.14% <78.12%> (+4.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08d0b31...2f45ee8. Read the comment docs.

Copy link
Owner

@pedromassango pedromassango left a 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.

@casvanluijtelaar
Copy link
Author

currently the showSelectedLabels and showUnselectedLabels options of the BottomNavigationBarThemeData are not implemented. Not something I'm planning on added in this pull request but might be an interesting feature where label visibility can be customised to your liking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants