-
Notifications
You must be signed in to change notification settings - Fork 499
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 #3844: Combine chapter listing double string to one string #4903
Fix #3844: Combine chapter listing double string to one string #4903
Conversation
@kkmurerwa is this ready for review? If so, you should "PTAL @<reviewer_name>" in order to kick-off the review process. |
@BenHenning it is ready for review. I hesitated to assign it to you because I assumed it automatically does it once you send a PR. Let me do that right now. |
PTAL @BenHenning |
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 @kkmurerwa! This looks really solid. Just had a few questions on clean-up, thoroughness, and testing--PTAL.
app/src/main/java/org/oppia/android/app/topic/lessons/StorySummaryViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/topic/lessons/StorySummaryViewModel.kt
Show resolved
Hide resolved
Hey @BenHenning. I am working on the issues you've identified. I will request for a review once that is done. |
…unt_with_story_name_as_one_string
… feature as it is no longer used
Hey @BenHenning. I have deleted the required files and updated the PR comment. |
PTAL @BenHenning |
Unassigning @kkmurerwa since a re-review was requested. @kkmurerwa, please make sure you have addressed all review comments. Thanks! |
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 @kkmurerwa! Just had one follow-up comment, but can you also update your PR description to reference the PR that originally removed the reference to computeStoryNameChapterCountContainerContentDescription
? That will help contextualize the "why this approach is correct" aspect of a clear PR description.
@BenHenning I have updated the PR comment. Please let me know if it is better now. |
PTAL @BenHenning |
Unassigning @kkmurerwa since a re-review was requested. @kkmurerwa, please make sure you have addressed all review comments. Thanks! |
Thanks @kkmurerwa. I followed up on the open comment--PTAL. |
app/src/main/java/org/oppia/android/app/topic/lessons/StorySummaryViewModel.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.
Thanks @kkmurerwa. This LGTM! I'll enable auto-merge so that you can merge this once CI checks are passing and your comment is resolved.
app/src/main/java/org/oppia/android/app/topic/lessons/StorySummaryViewModel.kt
Show resolved
Hide resolved
Hi @kkmurerwa, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Hi @BenHenning. I'll mark the remaining unresolved comment as resolved once all the checks pass. Thanks. |
Explanation
Fix #3844: When merged, this PR will;
R.plurals.chapter_count_with_story_name
from the strings file.R.plurals.chapter_count_with_story_name
.computeStoryNameChapterCountContainerContentDescription()
method fromStorySummaryViewModel
which is no longer used.The approach to delete the above files was necessitated by a change made on PR #4441. The PR switched from a chapter-based progress system i.e. Chapter 1 of 10 to a percentage-based progress system. This made the
chapter_count_with_story_name
strings and thecomputeStoryNameChapterCountContainerContentDescription()
method used to generate the chapter-based progress text redundant. These have been removed to eliminate this redundancy.Essential Checklist