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 #235: Change fragment implementation from directly xml code to better kotlin code. #238

Merged
merged 16 commits into from
Oct 16, 2019
Merged

Fix #235: Change fragment implementation from directly xml code to better kotlin code. #238

merged 16 commits into from
Oct 16, 2019

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Oct 15, 2019 •
edited
Loading

This PR fixes xml files that are using fragment tag directly to display a particular fragment.
It implements FrameLayout as container in xml and kotlin files initialize the corresponding fragment.

This change are implemented in following cases for now:

  • exploration_fragment.xml
  • state_fragment.xml

@veena14cs veena14cs changed the title Fix issue #42, #163: Change fragment implementation from directly xml code to better kotlin code. Fix #42 & #163: Change fragment implementation from directly xml code to better kotlin code. Oct 15, 2019
rt4914
rt4914 suggested changes Oct 15, 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.

PTAL @veena14cs

app/src/main/java/org/oppia/app/player/exploration/ExplorationActivity.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
@rt4914
Copy link
Contributor

rt4914 commented Oct 15, 2019 •
edited
Loading

@veena14cs
The title of this PR is incorrect. #42 and #163 are full implementation of some other work items.

You should reference this to its issue #235

@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Oct 15, 2019
@veena14cs veena14cs changed the title Fix #42 & #163: Change fragment implementation from directly xml code to better kotlin code. Fix #235: Change fragment implementation from directly xml code to better kotlin code. Oct 15, 2019
BenHenning
BenHenning approved these changes Oct 16, 2019
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks! Nice change. Changes LGTM once Rajat's comments are addressed.

@BenHenning BenHenning removed their assignment Oct 16, 2019
@veena14cs veena14cs requested a review from rt4914 October 16, 2019 05:09
@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Oct 16, 2019
rt4914
rt4914 approved these changes Oct 16, 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.

PTAL Nit change

app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentTest.kt Outdated Show resolved Hide resolved
nikitamarysolomanpvt
nikitamarysolomanpvt approved these changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

LGTM

@veena14cs veena14cs merged commit 36d52f3 into develop Oct 16, 2019
@veena14cs veena14cs deleted the state-fragment-fix branch October 16, 2019 06:44
@jamesxu0 jamesxu0 mentioned this pull request Oct 16, 2019
Merged
6 tasks
@BenHenning BenHenning mentioned this pull request Nov 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikitamarysolomanpvt nikitamarysolomanpvt nikitamarysolomanpvt approved these changes

@BenHenning BenHenning BenHenning approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@rt4914 rt4914

@nikitamarysolomanpvt nikitamarysolomanpvt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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