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

Fix part 135: Topic issues fixes #285

Merged
merged 97 commits into from
Nov 7, 2019
Merged

Fix part 135: Topic issues fixes #285

merged 97 commits into from
Nov 7, 2019

Conversation

nikitamarysolomanpvt
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt commented Oct 31, 2019 •
edited
Loading

Duplicate of #279

Explanation

  • On click of see-more in topic-overview will switch the tab to topic-play (with test case).

  • All tabs of topic will get topic-id and display data on the basis of topic-id coming from HomeActivity.

Pending (will be in next PR):

  • Removing dummy button on HomeFragment

  • Introducing TopicTestActivity and its corresponding presenter to load TopicFragment for a specific TOPIC_ID which will be used to test-code.

  • When user clicks the promoted story item in HomeFragment the story id will be passed to TopicActivity.If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a TopicPlay tab inside TopicPlayFragment and scroll to the specific item and expand it .

Mock

https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/0fad68f5-5ca4-4947-b8db-b4cf07235fd7/Topic-w-o-Play-19
To test this app, follow these steps:

  1. In AndroidManifest file make TopicActivity as launcher activity.

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 assigned to an appropriate reviewer.

@nikitamarysolomanpvt
multiple tabs
e2ed1c4
@nikitamarysolomanpvt
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
089ea5a 
… topic-player-multiple-tabs

# Conflicts:
#	app/build.gradle
#	app/src/main/AndroidManifest.xml
#	app/src/main/res/values/styles.xml
@nikitamarysolomanpvt
nit changes
43016b6
@nikitamarysolomanpvt
nit changes
3a1a350
@nikitamarysolomanpvt
nit changes
405a45e
@nikitamarysolomanpvt
nit changes
72c7360
@nikitamarysolomanpvt
nit changes
c5ffa61
@nikitamarysolomanpvt
nit changes
47e2efc
@nikitamarysolomanpvt
nit changes
b4ed9bb
@nikitamarysolomanpvt
nit changes
9dd39b9
@nikitamarysolomanpvt
nit changes
b2fca59
@nikitamarysolomanpvt
cust_args
f0cb60f
@nikitamarysolomanpvt
multiple tabs will getting topic id from home
2d21695 
Merge branch 'develop' of https://github.com/oppia/oppia-android into topic-player-multiple-tabs

# Conflicts:
#	app/src/main/AndroidManifest.xml
#	app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt
#	domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt
@nikitamarysolomanpvt
test cases,multiple tabs will getting topic id from home
93b889b 
Merge branch 'develop' of https://github.com/oppia/oppia-android into topic-player-multiple-tabs

# Conflicts:
#	app/src/main/AndroidManifest.xml
#	app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt
#	domain/src/main/java/org/oppia/domain/exploration/ExplorationRetriever.kt
@nikitamarysolomanpvt
nit
3130323
@nikitamarysolomanpvt
nit
bbe7cf3
@nikitamarysolomanpvt
nit
046e297
@nikitamarysolomanpvt
nit
ba506ab
@nikitamarysolomanpvt
nit
6f228d1
@nikitamarysolomanpvt
separate test cases for all four tabs
24be11b
@nikitamarysolomanpvt
nit
bfa2b78
@nikitamarysolomanpvt
nit
0c6d5d8
@nikitamarysolomanpvt
nit
f8f84f1
@nikitamarysolomanpvt
nit
efae9f7
@nikitamarysolomanpvt
nit
1ecfc2f
@nikitamarysolomanpvt
nit
293aa52
@nikitamarysolomanpvt
nit
949f89e
@nikitamarysolomanpvt
nit
71ae233
@nikitamarysolomanpvt
Topic Overview see more click and tab switch
423dd37
@nikitamarysolomanpvt
Topic Overview see more click and tab switch
d3458e1
rt4914
rt4914 suggested changes Nov 7, 2019
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.

This PR looks good to me, there are some changes from this PR as well as #230.

I am just expecting replies on my comment and then it would become easier to Approve.

Also, I wrote this comment on #230 #230 (review)

which was not addressed earlier but from this PR description, I think the points in the comment can be divided in two categories:

Finished:

  • Linking TopicActivity to Topic items of HomeFragment.
  • Transfer of TOPIC_ID from TopicFragment to ChildFragment is pending

Pending (mostly expected in next PR):

  • Removing dummy button on HomeFragment
  • Introducing TopicTestActivity and its corresponding presenter to load TopicFragment for a specific TOPIC_ID which will be used to test-code.
  • If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment

app/src/main/res/drawable/ic_overview_white_24dp.xml Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/review/TopicReviewFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/review/TopicReviewFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/play/TopicPlayFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/train/TopicTrainFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/topic/TopicActivity.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Nov 7, 2019
@rt4914
Copy link
Contributor

rt4914 commented Nov 7, 2019

This PR looks good to me, there are some changes from this PR as well as #230.

I am just expecting replies on my comment and then it would become easier to Approve.

Also, I wrote this comment on #230 #230 (review)

which was not addressed earlier but from this PR description, I think the points in the comment can be divided in two categories:

Finished:

  • Linking TopicActivity to Topic items of HomeFragment.
  • Transfer of TOPIC_ID from TopicFragment to ChildFragment is pending

Pending (mostly expected in next PR):

  • Removing dummy button on HomeFragment
  • Introducing TopicTestActivity and its corresponding presenter to load TopicFragment for a specific TOPIC_ID which will be used to test-code.
  • If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment

@nikitamarysolomanpvt Please reply on this too, whether the "PENDING" and "FINISHED" list is correct or not?

rt4914
rt4914 suggested changes Nov 7, 2019
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.

There are some comments which were resolved without fixing them, please fix them, and also, I am expecting your reply on my previous high level comment too.

@rt4914 rt4914 removed their assignment Nov 7, 2019
@nikitamarysolomanpvt
Copy link
Contributor Author

This PR looks good to me, there are some changes from this PR as well as #230.
I am just expecting replies on my comment and then it would become easier to Approve.
Also, I wrote this comment on #230 #230 (review)
which was not addressed earlier but from this PR description, I think the points in the comment can be divided in two categories:
Finished:

  • Linking TopicActivity to Topic items of HomeFragment.
  • Transfer of TOPIC_ID from TopicFragment to ChildFragment is pending

Pending (mostly expected in next PR):

  • Removing dummy button on HomeFragment
  • Introducing TopicTestActivity and its corresponding presenter to load TopicFragment for a specific TOPIC_ID which will be used to test-code.
  • If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment

@nikitamarysolomanpvt Please reply on this too, whether the "PENDING" and "FINISHED" list is correct or not?

It's updated in initial comment.

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 7, 2019
rt4914
rt4914 approved these changes Nov 7, 2019
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.

@nikitamarysolomanpvt LGTM, just resolve merge conflicts and then merge this.

@rt4914 rt4914 assigned nikitamarysolomanpvt and unassigned rt4914 Nov 7, 2019
@nikitamarysolomanpvt
Merge branches 'develop' and 'topic_issues_fixs' of https://github.co…
7e93850 
…m/oppia/oppia-android into topic_issues_fixs

# Conflicts:
#	app/src/main/res/values/strings.xml
@nikitamarysolomanpvt
Copy link
Contributor Author

There are some comments which were resolved without fixing them, please fix them, and also, I am expecting your reply on my previous high level comment too.

Done

@nikitamarysolomanpvt nikitamarysolomanpvt requested review from jamesxu0 and removed request for jamesxu0 November 7, 2019 09:45
@BenHenning BenHenning removed the request for review from jamesxu0 November 7, 2019 17:40
jamesxu0
jamesxu0 approved these changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@jamesxu0 jamesxu0 left a comment

Choose a reason for hiding this comment

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

LGTM

@nikitamarysolomanpvt nikitamarysolomanpvt merged commit 790fd64 into develop Nov 7, 2019
@rt4914 rt4914 mentioned this pull request Nov 8, 2019
Merged
6 tasks
@nikitamarysolomanpvt nikitamarysolomanpvt mentioned this pull request Nov 11, 2019
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning approved these changes

@jamesxu0 jamesxu0 jamesxu0 approved these changes

@veena14cs veena14cs veena14cs approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@nikitamarysolomanpvt nikitamarysolomanpvt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nikitamarysolomanpvt @veena14cs @rt4914 @BenHenning @jamesxu0