modified bottom-nav-bar #4489
modified bottom-nav-bar #4489
Conversation
|
|
|
@JuancaG05 Please provide a review. |
There was a problem hiding this comment.
Hi @Yogeshjindal! Thanks a lot for your contribution! 🍻 I just reviewed your PR, take a look at it, but I'll point out some comments as well:
- We have a specific naming for branches, so that our checks are triggered when the name of the branch follows this naming. I recommend taking a look at https://github.com/owncloud/android/blob/master/CONTRIBUTING.md so that you can follow our guidelines 👍
- Use conventional commits for naming. You can check more info here: https://github.com/owncloud/android/blob/master/CONTRIBUTING.md#2-create-pull-request. It's better if we follow the same guidelines, so that we can integrate it to our work more easily
- We add a Calens file in every PR so that the changelog is updated and we keep track of the changes made in the app. It's very easy! You can check how to do it and several examples here: https://github.com/owncloud/android/blob/master/changelog/README.md
- You can include a new release note for this issue, just as the check in the PR indicates. To do this, you just have to add a new
ReleaseNoteobject to the list inReleaseNoteViewModel.kt, creating new strings for that
Since there are several changes required and the content of the PR is not that big, I would suggest to close this PR and open a new one, it'll be quicker 😃. But it's up to you. If you decide not to close this one, remember to rename the branch and commit and use conventional commits from now on, also rebase the branch against master 👍
| app:activeIndicatorEnabled="true" | ||
| app:activeIndicatorColor="@color/active_indicator_color" |
There was a problem hiding this comment.
These 2 attributes seem not to exist, at least on the version of the BottomNavigationView that we're using. You can check this because the app build fails (take a look at the BitRise checks or check it locally in your computer)
| @@ -42,6 +42,7 @@ | |||
| <color name="actionbar_start_color">@color/primary</color> | |||
| <color name="primary_button_background_color">@color/color_accent</color> | |||
| <color name="primary_button_text_color">@color/white</color> | |||
| <color name="active_indicator_color">#3F51B5</color> | |||
There was a problem hiding this comment.
Why did you decide this color? We should keep consistency as far as we can with colors in the app 😃. In this case, it may even not be necessary, the active indicator is enabled by default (with the color already provided in app:itemTextColor), but maybe you can play with app:itemActiveIndicatorStyle to see different options
|
Hi @Yogeshjindal! Wanna try to open a new PR with the guidelines I provided and have your code included in our project? 😄 |
Hi @JuancaG05 Sir,I have submitted another pull request for this issue now. |
Added text labels to the bottom nav bar
Added active state (eg. "icon background pill")
Now, all the items in nav bar shows the labels and upon selection the activeIndicator is enabled which shows that which item is selected.
Fix#4484