Fix part of #135: UI Structure:Topic player multiple tabs #230

nikitamarysolomanpvt · 2019-10-14T11:21:33Z

Explanation

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:

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.

… topic-player-multiple-tabs # Conflicts: # app/build.gradle # app/src/main/AndroidManifest.xml # app/src/main/res/values/styles.xml

nikitamarysolomanpvt · 2019-10-17T09:46:17Z

@BenHenning Please confirm the approach. Please do suggest the test case for multiple tabs.

rt4914 · 2019-10-17T17:29:22Z

@nikitamarysolomanpvt
Revise title:

Fix #135 :UI Structure:Topic player multiple tabs

rt4914

LGTM.
Left some nit changes and add test-cases.

Also @BenHenning PTAL, basically can you verify this approach first so that @nikitamarysolomanpvt can start working on test-cases for this PR.

app/src/main/AndroidManifest.xml

app/build.gradle

app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt

BenHenning

Thanks @nikitamarysolomanpvt! Can you add test cases? E.g. it would be nice to see TopicFragment tests for each of the four tabs that:

Verify the presence of the tab
Verify navigating to the tab shows the correct content
Verify that a configuration change keeps the same tab open

And one test for verifying that the initial tab that's open by default is correct for a total of 13 tests.

app/src/main/AndroidManifest.xml

app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt

app/src/main/res/layout/topic_fragment.xml

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

rt4914 · 2019-10-24T11:28:45Z

@nikitamarysolomanpvt Search for TODO(#135) in code which can be fixed in your PR.

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

rt4914

LGTM

app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt

rt4914 · 2019-11-06T05:16:11Z

@nikitamarysolomanpvt Because of latest code merge your PR now has merge conflicts, please resolve them.

nikitamarysolomanpvt · 2019-11-06T06:34:04Z

@nikitamarysolomanpvt Because of latest code merge your PR now has merge conflicts, please resolve them.

Merged with develop

BenHenning

Thanks @nikitamarysolomanpvt--really solid tests!

app/src/main/java/org/oppia/app/topic/TopicActivity.kt

app/src/main/java/org/oppia/app/topic/TopicFragment.kt

app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt

app/src/main/java/org/oppia/app/topic/TopicTab.kt

app/src/sharedTest/java/org/oppia/app/utility/TabMatcher.kt

app/src/sharedTest/java/org/oppia/app/topic/TopicFragmentTest.kt

BenHenning

Thanks @nikitamarysolomanpvt -- the PR generally LGTM, and I'm deferring on @rt4914 to verify that the specific concerns I brought up in my comments are addressed. Approving so that this can be checked in later today.

nikitamarysolomanpvt · 2019-11-06T07:23:43Z

Thanks @nikitamarysolomanpvt -- the PR generally LGTM, and I'm deferring on @rt4914 to verify that the specific concerns I brought up in my comments are addressed. Approving so that this can be checked in later today.

I would like to make the new suggested changes in the #285. Because #285 is dependent on #230 as well as it is base branch of #285. If you want i can create issue on the same.

rt4914

LGTM, there are still nit change which needs to be fixed.

Also, I suggest before merging this PR mention one comment regarding what all work which was not finished in this PR, so that anyone going through this PR in future has clear context of our verbal conversations.

Sample:

Removing dummy button from home and linking TopicActivity to Topic items of HomeFragment.
Introducing TopicTestActivity and its corresponding presenter to load TopicFragment for a specific TOPIC_ID which will be used to test-code.
Transfer of TOPIC_ID from TopicFragment to ChildFragment is pending
If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment

If you think something else is missing feel free to add that too.

app/src/main/java/org/oppia/app/topic/TopicActivity.kt

rt4914 · 2019-11-06T12:20:53Z

app/src/main/java/org/oppia/app/topic/TopicFragment.kt


 override fun onAttach(context: Context?) {
 super.onAttach(context)
 fragmentComponent.inject(this)
 }

 override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? {
- return topicFragmentPresenter.handleCreateView(inflater, container)
+ topicId = arguments?.getString(TOPIC_ID_ARGUMENT_KEY) ?: TEST_TOPIC_ID_0


NOTE: TEST_TOPIC_ID_0 will be removed in either #285 or a separate PR. For now we need this successfully running the test-cases.

app/src/main/java/org/oppia/app/topic/ViewPagerAdapter.kt

app/src/main/res/drawable/ic_overview_white_24dp.xml

app/src/sharedTest/java/org/oppia/app/topic/TopicFragmentTest.kt

app/src/main/java/org/oppia/app/topic/TopicTab.kt

app/src/sharedTest/java/org/oppia/app/utility/TabMatcher.kt

veena14cs

LGTM.

rt4914 · 2019-11-06T17:08:43Z

LGTM, there are still nit change which needs to be fixed.

Also, I suggest before merging this PR mention one comment regarding what all work which was not finished in this PR, so that anyone going through this PR in future has clear context of our verbal conversations.

Sample:

Removing dummy button from home and linking TopicActivity to Topic items of HomeFragment.

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

Transfer of TOPIC_ID from TopicFragment to ChildFragment is pending

If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment

If you think something else is missing feel free to add that too.

@nikitamarysolomanpvt I think you missed out on this comment before merging the PR.

nikitamarysolomanpvt · 2019-11-07T07:34:53Z

Also, I suggest before merging this PR mention one comment regarding what all work which was not finished in this PR, so that anyone going through this PR in future has clear context of our verbal conversations.

Sample:

Removing dummy button from home and linking TopicActivity to Topic items of HomeFragment.

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

Transfer of TOPIC_ID from TopicFragment to ChildFragment is pending

If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment

If you think something else is missing feel free to add that too.

Sorry i missed this comment.
Adding a few more

If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment and expand the item.
If the TOPIC_ID and STORY_ID is present in TopicActivity then the UI should traverse to a specific STORY inside TopicPlayFragment (resolved in Fix part 135: Topic issues fixes #285).

nikitamarysolomanpvt added 2 commits October 7, 2019 10:46

multiple tabs

e2ed1c4

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 changed the title ~~Topic player multiple tabs~~ fix #135 :UI Structure:Topic player multiple tabs Oct 14, 2019

nikitamarysolomanpvt added 9 commits October 17, 2019 11:44

nit changes

43016b6

nit changes

3a1a350

nit changes

405a45e

nit changes

72c7360

nit changes

c5ffa61

nit changes

47e2efc

nit changes

b4ed9bb

nit changes

9dd39b9

nit changes

b2fca59

nikitamarysolomanpvt requested review from rt4914 and veena14cs October 17, 2019 09:17

nikitamarysolomanpvt assigned veena14cs, rt4914 and BenHenning Oct 17, 2019

rt4914 reviewed Oct 17, 2019

View reviewed changes

app/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved

app/build.gradle Outdated Show resolved Hide resolved

app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt Outdated Show resolved Hide resolved

rt4914 assigned nikitamarysolomanpvt and unassigned rt4914 Oct 17, 2019

cust_args

f0cb60f

BenHenning changed the title ~~fix #135 :UI Structure:Topic player multiple tabs~~ Fix #135: UI Structure:Topic player multiple tabs Oct 21, 2019

BenHenning requested changes Oct 21, 2019

View reviewed changes

BenHenning removed their assignment Oct 21, 2019

veena14cs reviewed Oct 23, 2019

View reviewed changes

app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved

veena14cs removed their assignment Oct 23, 2019

rt4914 approved these changes Nov 5, 2019

View reviewed changes

app/src/main/java/org/oppia/app/topic/TopicFragmentPresenter.kt Outdated Show resolved Hide resolved

rt4914 removed their assignment Nov 5, 2019

nikitamarysolomanpvt added 2 commits November 6, 2019 11:52

merged with develop

83ad843

nit

1aa955e

BenHenning requested changes Nov 6, 2019

View reviewed changes

BenHenning assigned nikitamarysolomanpvt and unassigned BenHenning Nov 6, 2019

BenHenning approved these changes Nov 6, 2019

View reviewed changes

changes suggested by ben #230 (review)

ba16385

nikitamarysolomanpvt assigned rt4914 and veena14cs and unassigned nikitamarysolomanpvt Nov 6, 2019

nikitamarysolomanpvt added 2 commits November 6, 2019 17:24

nit

236384f

nit

8d6454f

rt4914 approved these changes Nov 6, 2019

View reviewed changes

rt4914 assigned nikitamarysolomanpvt and unassigned rt4914 Nov 6, 2019

nit

898eadf

nikitamarysolomanpvt removed their assignment Nov 6, 2019

veena14cs approved these changes Nov 6, 2019

View reviewed changes

veena14cs assigned nikitamarysolomanpvt and unassigned veena14cs Nov 6, 2019

nikitamarysolomanpvt merged commit d127798 into develop Nov 6, 2019

rt4914 mentioned this pull request Nov 7, 2019

TopicPlayFragment: Rigorous click on dropdown icon results in unexpected behaviour #286

Closed

nikitamarysolomanpvt deleted the topic-player-multiple-tabs branch November 14, 2019 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #135: UI Structure:Topic player multiple tabs #230

Fix part of #135: UI Structure:Topic player multiple tabs #230

nikitamarysolomanpvt commented Oct 14, 2019 •

edited

Loading

nikitamarysolomanpvt commented Oct 17, 2019

rt4914 commented Oct 17, 2019

rt4914 left a comment

BenHenning left a comment

rt4914 commented Oct 24, 2019

rt4914 left a comment

rt4914 commented Nov 6, 2019

nikitamarysolomanpvt commented Nov 6, 2019 •

edited

Loading

BenHenning left a comment

BenHenning left a comment

nikitamarysolomanpvt commented Nov 6, 2019

rt4914 left a comment

rt4914 Nov 6, 2019

veena14cs left a comment

rt4914 commented Nov 6, 2019 •

edited

Loading

nikitamarysolomanpvt commented Nov 7, 2019

Fix part of #135: UI Structure:Topic player multiple tabs #230

Fix part of #135: UI Structure:Topic player multiple tabs #230

Conversation

nikitamarysolomanpvt commented Oct 14, 2019 • edited Loading

Explanation

Mock

Checklist

nikitamarysolomanpvt commented Oct 17, 2019

rt4914 commented Oct 17, 2019

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

rt4914 commented Oct 24, 2019

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 commented Nov 6, 2019

nikitamarysolomanpvt commented Nov 6, 2019 • edited Loading

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

nikitamarysolomanpvt commented Nov 6, 2019

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 Nov 6, 2019

Choose a reason for hiding this comment

veena14cs left a comment

Choose a reason for hiding this comment

rt4914 commented Nov 6, 2019 • edited Loading

nikitamarysolomanpvt commented Nov 7, 2019

nikitamarysolomanpvt commented Oct 14, 2019 •

edited

Loading

nikitamarysolomanpvt commented Nov 6, 2019 •

edited

Loading

rt4914 commented Nov 6, 2019 •

edited

Loading