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

Feature/add text labels bottom nav bar #4497

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

Conversation

Yogeshjindal
Copy link

Added text labels to the bottom nav bar
Used default activeStateIndicator which will show which label is selected

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

@JuancaG05 JuancaG05 self-requested a review October 11, 2024 11:50
@JuancaG05 JuancaG05 linked an issue Oct 11, 2024 that may be closed by this pull request
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Hey @Yogeshjindal! Thanks for this new PR! Let's take a look at the points I highlighted in the previous one:

  • Branch naming is perfect! ✅
  • Conventional commits are not well used here. There are 4 commits, of which 3 are not useful. We only need the second one, that should be named feat: added text labels and active indicator to BottomNavigationView (for example) to follow the convention. I recommend removing those 3 non-necessary commits to avoid confusions (see the comment below), seems you were importing some Material stuff that you didn't need in the end 👍.
  • Check the comments I added about the Calens file and fix it 😄
  • There's also a release note to add. To do this, you just have to add a new ReleaseNote object to the list in ReleaseNoteViewModel.kt, creating new strings for that

Feel free to open a new PR or correcting it in this same branch. In the next comment I'll show you how to remove commits 👍

changelog/unreleased/4484 Outdated Show resolved Hide resolved
changelog/unreleased/4484 Outdated Show resolved Hide resolved
changelog/unreleased/4484 Outdated Show resolved Hide resolved
owncloudApp/build.gradle Outdated Show resolved Hide resolved
@JuancaG05
Copy link
Collaborator

A quick explanation about removing commits so that you learn for a future too:

  1. Using the Terminal from Android Studio, run the command:
git rebase -i HEAD~5

The number that goes together with HEAD is the number of commits you want to work with, but since this PR has 4, 5 is enough

  1. Replace pick by drop in those commits you want to discard (remember this is a vim editor, you have to use arrows to move, i to enter writing mode and Esc to exit the writing mode)
  2. Exit the writing mode, write :wq and press Enter
  3. Run the command to push it to GitHub (-f because otherwise you won't be able to push it since it will detect conflicts):
git push -f

After this, you should have in this PR only the commits we want and I'll be able to review it again 😊

@Yogeshjindal
Copy link
Author

@JuancaG05 Sir,Thanks a lot for sharing this valuable information.

Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Hey @Yogeshjindal, check this new CR round I made. Conventional commits are not correctly used yet, I insist on following the instructions I posted in the message above to remove commits that are not needed 👍.
Also, this needs a rebase against master, there was new content added there.

@@ -832,5 +832,7 @@
<string name="audio_preview_label">Audio preview</string>
<string name="details_label">Details</string>
<string name="text_preview_label">Text preview</string>
<string name="release_notes_title_enhanced_bottom_nav_bar">Added text labels on bottom nav bar</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to be read by all users, so let's not use technical language. Bottom bar is OK 👍

Suggested change
<string name="release_notes_title_enhanced_bottom_nav_bar">Added text labels on bottom nav bar</string>
<string name="release_notes_title_enhanced_bottom_nav_bar">Added text labels on bottom bar</string>

@@ -832,5 +832,7 @@
<string name="audio_preview_label">Audio preview</string>
<string name="details_label">Details</string>
<string name="text_preview_label">Text preview</string>
<string name="release_notes_title_enhanced_bottom_nav_bar">Added text labels on bottom nav bar</string>
<string name="release_notes_subtitle_bottom_nav_bar">Text labels were added and default active indicator is used to show which label is selected on the bottom nav bar</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<string name="release_notes_subtitle_bottom_nav_bar">Text labels were added and default active indicator is used to show which label is selected on the bottom nav bar</string>
<string name="release_notes_subtitle_bottom_nav_bar">Text labels were added and default active indicator is used to show which section is selected on the bottom bar</string>

@@ -34,7 +34,7 @@ dependencies {
implementation libs.androidx.work.runtime.ktx
implementation(libs.androidx.browser) { because "CustomTabs required for OAuth2 and OIDC" }
implementation(libs.androidx.enterprise.feedback) { because "MDM feedback" }

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are keeping this change, it's not needed

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.

[FEATURE REQUEST] Add text labels to bottom nav bar
2 participants