-
Notifications
You must be signed in to change notification settings - Fork 606
Fix #6042 Drawer highlight issue #6045
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
base: develop
Are you sure you want to change the base?
Conversation
|
@adhiamboperes I think this CI failure appears unrelated to this PR. |
adhiamboperes
left a comment
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 @itsadityaaaaa!
There are a few things to clarify/improve:
- Part of the PR description reads: "Removed dependency on menu ordering, preventing issues when menu structure changes." However, I see no code that points to this. Could you please elaborate?
- The NavigationDrawerFragment.kt is covered via
NavigationDrawerActivityDebugTestandNavigationDrawerActivityProdTest. Please add tests for verifying the changes introduced. If you need support on a way forward, please let us know. - The video demos show menu highlight for the options items, but it is important to show that this is fixed for the footer items as weel, i.e. the admin controls and developer options items.
- The PR title and the start of the explanation are missing a reference to "fix #issueNumber", which are used to automate closing the issue once the PR is merged. Please add these.
|
@adhiamboperes Thank You! for your suggestions, will work on it and get back to you if need any help |
Coverage ReportResultsNumber of files assessed: 2 Exempted coverageFiles exempted from coverage
|
|
@adhiamboperes I did changes according to the suggestion, please review the modification and tell me any other modifications are required. |
|
@adhiamboperes Is there anything else I need to do on this PR? |
Explanation
Essential Checklist
For UI-specific PRs only
Tablet_demo.mp4
mobile_demo.mp4
If your PR includes UI-related changes, then: