Fix #22: Introduce navigation drawer #84

nikitamarysolomanpvt · 2019-08-29T14:59:55Z

Explanation

Fragment Navigation drawer
as per design document
https://docs.google.com/document/d/1Q9957ri3TlsTWdwBVAjn4g9EJzqmzBqrSXCHI3yRTEE/edit?usp=sharing
make NavigationDrawerTestActivity as launcher activity in Manifest file for checking test-cases.

Mock

https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/346dd03d-464b-4b49-9d7f-7ec6264fefd6/TP-Play-Side-Menu-1

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

https://docs.google.com/document/d/1Q9957ri3TlsTWdwBVAjn4g9EJzqmzBqrSXCHI3yRTEE/edit?usp=sharing #logout svg and profile image place holder pending

seanlip · 2019-08-29T17:42:41Z

@nikitamarysolomanpvt I'm assuming this is ready for review? If so please make sure to assign a reviewer on GitHub so that they know that they need to look at it. (And, if not, please explain that clearly in your PR description as well as a full list of "what is left to do" before the PR can be reviewed.)

Thanks!

BenHenning · 2019-08-30T05:09:50Z

@nikitamarysolomanpvt it appears your base branch is oppia:develop, but we want to merge into oppia-android:develop. Please update the PR base and rebase your branch, if needed.

Regarding the changes, please split this up. We should first check in the navigation drawer with one specific item, and be sure that we like how that solution looks before replicating it for other pages.

Can you also add screenshots to this PR for what this navigation drawer looks like in the app? That will better help contextualize the code for reviewers.

Regarding the design doc, please aim to have the design doc reviewed and signed off by someone on the team before creating your PR. That helps provide team members with time to comment on your approach before all of the code is written. This is meant to help save you time as well, since ideally design docs should be written with little code to get early feedback on your approach rather than duplicating the work of the pull request.

nikitamarysolomanpvt · 2019-08-30T14:38:31Z

@BenHenning even though it is showing oppia:develop on click of it it is taking us to oppia-android:develop not able to find why is it showing oppia:develop .Please find the screenshot of the developed navigation drawer.
The changes instructed by you today is still under development .

BenHenning · 2019-09-01T21:39:39Z

Since this is actually the navigation drawer, I updated the title accordingly. This may actually fully fix #22--it's not yet clear to me.

BenHenning · 2019-09-01T21:40:37Z

Thanks @nikitamarysolomanpvt, I'm not sure why the base branch is shown differently here but it appears to be the correct branch.

Edit: Ah, I think it's actually because you forked oppia-android, and it's shortening the repository names to project:branch rather than project/repo:branch.

…ple code https://github.com/nikitamarysolomanpvt/navigation/blob/master/KotlinDrawerBasic/app/src/main/java/com/example/parsaniahardik/kotlindrawerbasic/MainActivity.kt

…ies , ParentActivity for redusing boilerplates , On menu item clicks open corresponding activities(not fragments)on the basis of selected ids. animation for activity transitions

nikitamarysolomanpvt · 2019-09-03T19:44:27Z

Code is updated with new approach in navigation drawer .Using navigation drawer inside fragment ,drawerFragment can be reused with other activities which needs navigation drawer,ParentActivity for reducing boilerplates ,On menu item clicks open corresponding activities(not fragments)on the basis of selected ids.
animation for activity transitions

.idea/codeStyles/Project.xml

.idea/misc.xml

app/build.gradle

app/src/main/AndroidManifest.xml

app/src/main/res/menu/activity_navigation_drawer_drawer.xml

app/src/main/res/values-v21/styles.xml

app/src/main/res/values/strings.xml

app/src/main/res/values/styles.xml

build.gradle

nikitamarysolomanpvt

Please address some comments and assign it back.

Done

veena14cs

LGTM

rt4914 · 2019-12-19T04:34:48Z

@nikitamarysolomanpvt Please check these comments
#84 (comment)
#84 (comment)

…m/oppia/oppia-android into navigation-drawer # Conflicts: # app/src/main/AndroidManifest.xml # app/src/main/java/org/oppia/app/activity/ActivityComponent.kt # app/src/main/java/org/oppia/app/fragment/FragmentComponent.kt # app/src/main/java/org/oppia/app/home/HomeActivity.kt

rt4914 · 2019-12-20T07:28:24Z

I think we can assign it to @BenHenning now.

BenHenning

Thanks @nikitamarysolomanpvt! Overall this looks really good to me. I have a few suggestions around readability and maintainability, but they are effectively nits.

app/src/main/java/org/oppia/app/drawer/NavigationDrawerItem.kt

BenHenning · 2019-12-23T08:33:47Z

app/src/sharedTest/java/org/oppia/app/testing/NavigationDrawerTestActivityTest.kt

+
+/** Tests for [NavigationDrawerTestActivity]. */
+@RunWith(AndroidJUnit4::class)
+class NavigationDrawerTestActivityTest {


Although the prod activities don’t support landscape currently, can you also add config change tests to make sure the desired behavior happens? Eg that the drawer works correctly in landscape for at least one test scenario, and that rotating closes the drawer?

@BenHenning Added test cases for landscape mode test.
Opened drawer stays open on orientation change, rotating dose not closes the drawer ... do you want me to change this?

Hmm. Let’s keep that behavior. Despite my earlier comment, I don’t have a compelling reason for closing the drawer on config changes. I can see either scenario as being okay.

rt4914

LGTM. just one nit change suggested.

app/src/main/java/org/oppia/app/drawer/NavigationDrawerItem.kt

nikitamarysolomanpvt added 3 commits August 29, 2019 11:03

first commit

6214ff0

navigation drawer as per design doc

d6e4f22

https://docs.google.com/document/d/1Q9957ri3TlsTWdwBVAjn4g9EJzqmzBqrSXCHI3yRTEE/edit?usp=sharing #logout svg and profile image place holder pending

navigation drawer as per design doc

b89da5d

https://docs.google.com/document/d/1Q9957ri3TlsTWdwBVAjn4g9EJzqmzBqrSXCHI3yRTEE/edit?usp=sharing #logout svg and profile image place holder pending

seanlip requested a review from BenHenning August 29, 2019 17:41

seanlip assigned BenHenning Aug 29, 2019

BenHenning assigned nikitamarysolomanpvt and unassigned BenHenning Aug 30, 2019

nikitamarysolomanpvt changed the title ~~Navigation drawer~~ Fix part of #43 Fragment & activity navigation guidelines #43 Aug 30, 2019

nikitamarysolomanpvt changed the title ~~Fix part of #43 Fragment & activity navigation guidelines #43~~ Fix part of #43 : Fragment & activity navigation guidelines Aug 30, 2019

BenHenning changed the title ~~Fix part of #43 : Fragment & activity navigation guidelines~~ Fix part of #22: Introduce navigation drawer Sep 1, 2019

nikitamarysolomanpvt added 3 commits September 3, 2019 15:37

fragment transaction in container activity and drawer in fragment sam…

831ddee

…ple code https://github.com/nikitamarysolomanpvt/navigation/blob/master/KotlinDrawerBasic/app/src/main/java/com/example/parsaniahardik/kotlindrawerbasic/MainActivity.kt

Navigation drawer with drawerFragment for reusing for several activit…

73eaa29

…ies , ParentActivity for redusing boilerplates , On menu item clicks open corresponding activities(not fragments)on the basis of selected ids. animation for activity transitions

Navigation drawer with drawerFragment for reusing for several activit…

a01aa9b

…ies , ParentActivity for redusing boilerplates , On menu item clicks open corresponding activities(not fragments)on the basis of selected ids. animation for activity transitions

nikitamarysolomanpvt assigned BenHenning, droidizer, rt4914, seanlip and veena14cs and unassigned nikitamarysolomanpvt Sep 4, 2019

rt4914 suggested changes Sep 4, 2019

View reviewed changes

rt4914 unassigned droidizer, rt4914, seanlip and BenHenning Sep 4, 2019

nikitamarysolomanpvt commented Dec 18, 2019

View reviewed changes

nit changes

5d1417f

nikitamarysolomanpvt assigned rt4914 and veena14cs and unassigned nikitamarysolomanpvt Dec 18, 2019

updated test-cases

01e6bfd

veena14cs approved these changes Dec 18, 2019

View reviewed changes

veena14cs assigned nikitamarysolomanpvt and unassigned veena14cs Dec 18, 2019

rt4914 removed their assignment Dec 19, 2019

nikitamarysolomanpvt mentioned this pull request Dec 19, 2019

@Inject related reformatting issues #568

Closed

nikitamarysolomanpvt assigned BenHenning and unassigned nikitamarysolomanpvt Dec 19, 2019

nikitamarysolomanpvt added 2 commits December 20, 2019 12:20

nit

71f6063

nikitamarysolomanpvt assigned rt4914 Dec 20, 2019

rt4914 removed their assignment Dec 20, 2019

BenHenning approved these changes Dec 23, 2019

View reviewed changes

BenHenning assigned nikitamarysolomanpvt and unassigned BenHenning Dec 23, 2019

NavigationDrawerItem enum updated and testcase added for Landscape Mode

ee3a0a3

nikitamarysolomanpvt assigned rt4914 and unassigned nikitamarysolomanpvt Dec 23, 2019

rt4914 approved these changes Dec 24, 2019

View reviewed changes

app/src/main/java/org/oppia/app/drawer/NavigationDrawerItem.kt Outdated Show resolved Hide resolved

rt4914 assigned nikitamarysolomanpvt and unassigned rt4914 Dec 24, 2019

nit

50de44e

nikitamarysolomanpvt merged commit 2554314 into oppia:develop Dec 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #22: Introduce navigation drawer #84

Fix #22: Introduce navigation drawer #84

nikitamarysolomanpvt commented Aug 29, 2019 •

edited

Loading

seanlip commented Aug 29, 2019 •

edited

Loading

BenHenning commented Aug 30, 2019

nikitamarysolomanpvt commented Aug 30, 2019

BenHenning commented Sep 1, 2019

BenHenning commented Sep 1, 2019 •

edited

Loading

nikitamarysolomanpvt commented Sep 3, 2019

nikitamarysolomanpvt left a comment

veena14cs left a comment

rt4914 commented Dec 19, 2019

rt4914 commented Dec 20, 2019

BenHenning left a comment

BenHenning Dec 23, 2019

nikitamarysolomanpvt Dec 23, 2019

BenHenning Dec 24, 2019

rt4914 left a comment

Fix #22: Introduce navigation drawer #84

Fix #22: Introduce navigation drawer #84

Conversation

nikitamarysolomanpvt commented Aug 29, 2019 • edited Loading

Explanation

Mock

Checklist

seanlip commented Aug 29, 2019 • edited Loading

BenHenning commented Aug 30, 2019

nikitamarysolomanpvt commented Aug 30, 2019

BenHenning commented Sep 1, 2019

BenHenning commented Sep 1, 2019 • edited Loading

nikitamarysolomanpvt commented Sep 3, 2019

nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

veena14cs left a comment

Choose a reason for hiding this comment

rt4914 commented Dec 19, 2019

rt4914 commented Dec 20, 2019

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Dec 23, 2019

Choose a reason for hiding this comment

nikitamarysolomanpvt Dec 23, 2019

Choose a reason for hiding this comment

BenHenning Dec 24, 2019

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

nikitamarysolomanpvt commented Aug 29, 2019 •

edited

Loading

seanlip commented Aug 29, 2019 •

edited

Loading

BenHenning commented Sep 1, 2019 •

edited

Loading