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.

Sign up for GitHub

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

Jump to bottom

Fixs #654: Add Navigation Drawer Icon Topic #741

Merged
merged 23 commits into from
Mar 19, 2020
Merged

Fixs #654: Add Navigation Drawer Icon Topic #741

merged 23 commits into from
Mar 19, 2020

Conversation

PrarabdhGarg
Copy link
Contributor

Explanation

Fixes #654

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.

@PrarabdhGarg
Merge pull request #1 from oppia/develop
710093a 
Sync forked repository
@PrarabdhGarg
Merge pull request #2 from oppia/develop
dc7bbbb 
Merge
@PrarabdhGarg
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
043184f 
… develop
@PrarabdhGarg
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
48b2083 
… develop
@PrarabdhGarg
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
b1ef48f 
… develop
@PrarabdhGarg
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
e4d543b 
… develop
@PrarabdhGarg
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
c39cc4c 
… develop
@PrarabdhGarg
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
e4e748d 
… develop
@PrarabdhGarg
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
df1ee11 
… develop
@PrarabdhGarg
Move toolbar from fragment to activity
47d34f8
@PrarabdhGarg
Add listener to get title from fragments in activity
556a7d9
@PrarabdhGarg
Remove MergeConflict
f71f87a
@PrarabdhGarg
Add EOF at TopicTitleListener
5a5f74e
@PrarabdhGarg PrarabdhGarg requested a review from rt4914 March 5, 2020 10:08
@PrarabdhGarg
Copy link
Contributor Author

@rt4914 I still have to make changes to the test cases. But could you just confirm if the implementation of the listener is correct

rt4914
rt4914 suggested changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

PTAL

app/src/main/java/org/oppia/app/topic/ToolbarTitleListener.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned PrarabdhGarg and unassigned rt4914 Mar 5, 2020
@PrarabdhGarg PrarabdhGarg requested a review from rt4914 March 7, 2020 08:29
@PrarabdhGarg PrarabdhGarg assigned rt4914 and unassigned PrarabdhGarg Mar 9, 2020
@rt4914 rt4914 assigned PrarabdhGarg and unassigned rt4914 Mar 9, 2020
@PrarabdhGarg PrarabdhGarg assigned rt4914 and unassigned PrarabdhGarg Mar 9, 2020
@rt4914 rt4914 assigned PrarabdhGarg and unassigned rt4914 Mar 9, 2020
@rt4914
Copy link
Contributor

rt4914 commented Mar 9, 2020

Screenshot_1583750032

@PrarabdhGarg Tabs are not visible in current implementation.

@rt4914 rt4914 assigned PrarabdhGarg and unassigned rt4914 Mar 9, 2020
@PrarabdhGarg
Copy link
Contributor Author

@rt4914 I am unable to find the error in my present code. Could you suggest what could be a possible reason for this error?

@PrarabdhGarg PrarabdhGarg assigned rt4914 and unassigned PrarabdhGarg Mar 11, 2020
@PrarabdhGarg
Copy link
Contributor Author

@rt4914 I have made the changes

rt4914
rt4914 suggested changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Looks good, some nit changes and suggestion on test cases.

app/src/main/java/org/oppia/app/topic/TopicActivityPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/TopicActivityPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/topic_activity.xml Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/topic/TopicFragmentTest.kt Show resolved Hide resolved
@rt4914 rt4914 assigned PrarabdhGarg and unassigned rt4914 Mar 11, 2020
@rt4914
Copy link
Contributor

rt4914 commented Mar 18, 2020

@PrarabdhGarg If you still. busy. maybe we can close this issue so that someone else can work on this?

@PrarabdhGarg
Copy link
Contributor Author

@rt4914 I will finish this by today

@PrarabdhGarg PrarabdhGarg assigned rt4914 and unassigned PrarabdhGarg Mar 19, 2020
rt4914
rt4914 suggested changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If you notice correctly, the navigation drawer is not showing any profile name.

This is because of the fact that the internalProfileId is not getting passed correctly in NavigationDrawerFragment.

This can be solved by change doing following steps in TopicActivity
Remove TOPIC_ACTIVITY_INTERNAL_PROFILE_ID_ARGUMENT_KEY and replace it with KEY_NAVIGATION_PROFILE_ID

@rt4914 rt4914 assigned PrarabdhGarg and unassigned rt4914 Mar 19, 2020
rt4914
rt4914 suggested changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

PTAL

app/src/main/java/org/oppia/app/topic/TopicActivityPresenter.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/topic/TopicActivityTest.kt Show resolved Hide resolved
@PrarabdhGarg PrarabdhGarg assigned rt4914 and unassigned PrarabdhGarg Mar 19, 2020
rt4914
rt4914 approved these changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@rt4914 rt4914 merged commit f1bd53b into oppia:develop Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Topic should have navigation drawer icon in it
2 participants
@PrarabdhGarg @rt4914