-
Notifications
You must be signed in to change notification settings - Fork 503
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.
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
Fix part of #137: Added story-progress in Topic-Play-Tab #262
Conversation
} | ||
.size | ||
|
||
val storyProgressPercentage: Int = (completedChapterCount / totalChapterCount) * 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit change: Please remove explicit type specifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we need this type to make sure that the result of right-side is always in integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this division work correctly? It seems like an int divided by an int--won't this always result in 0? I think you need to convert to a float somewhere before converting back to int for display.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nit changes and test case failing PTAL
app/src/sharedTest/java/org/oppia/app/topic/play/TopicPlayFragmentTest.kt
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/topic/play/TopicPlayFragmentTest.kt
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/app/topic/play/TopicPlayFragmentTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM please check test case.
app/src/sharedTest/java/org/oppia/app/topic/play/TopicPlayFragmentTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rt4914. Approach LGTM, but I think there's one bug that needs to be addressed here.
} | ||
.size | ||
|
||
val storyProgressPercentage: Int = (completedChapterCount / totalChapterCount) * 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this division work correctly? It seems like an int divided by an int--won't this always result in 0? I think you need to convert to a float somewhere before converting back to int for display.
app/src/sharedTest/java/org/oppia/app/topic/play/TopicPlayFragmentTest.kt
Outdated
Show resolved
Hide resolved
The progress percentage calculation was incorrect as rightly pointed out by @BenHenning. I identified this issue when I was working on Expandable List and solved it there and missed on fixing that in this PR. Fixing it now. |
Explanation
This PR is in continuation with #260 and implements the story-progress in topic-play-tab.
To test this functionality make following changes:
TopicActivity
as launcher activity.TopicActivity
should showTopicPlayFragment
instead ofTopicFragment
. (Do this change inTopicActivityPresenter
)Checklist