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

Fixes #3169: Item selection interaction text change on selection #4481

Closed
wants to merge 4 commits into from
Closed

Fixes #3169: Item selection interaction text change on selection #4481

wants to merge 4 commits into from

Conversation

Akshatkamboj14
Copy link
Member

Explanation

Fixes: #3169

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

rt4914
rt4914 reviewed Aug 2, 2022
View reviewed changes
.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt
}
selectedItems.size == maxAllowableSelectionCount -> {
selectedItems += itemIndex
selectedItemText.set("No more than $maxAllowableSelectionCount choices may be selected.")
updateIsAnswerAvailable()
true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Create a separate function which accepts size and maxAllowableSelectionCount which you can use to set the correct string.

Now decide where do want to use that function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @rt4914 I have pushed the same, PTAL.

@Akshatkamboj14 Akshatkamboj14 requested a review from rt4914 August 5, 2022 20:15
@oppiabot
Copy link

oppiabot bot commented Aug 12, 2022

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 12, 2022
@Akshatkamboj14 Akshatkamboj14 marked this pull request as ready for review August 13, 2022 09:26
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 13, 2022
@BenHenning BenHenning self-assigned this Aug 19, 2022
BenHenning
BenHenning requested changes Aug 24, 2022
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 @Akshatkamboj14. Sorry for the delayed review.

I took an initial review pass and left a few comments. Besides those, please also make sure to add tests to verify that the correct wording is used at the correct time for the interaction.

.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt
@@ -148,6 +152,18 @@ class SelectionInteractionViewModel private constructor(
}
}

private fun updateSelectionText() {
Copy link
Member

Choose a reason for hiding this comment

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

Just to check--do these wordings match Oppia web?

.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt
@@ -56,6 +56,7 @@ class SelectionInteractionViewModel private constructor(
computeChoiceItems(choiceSubtitledHtmls, hasConversationView, this)

private val isAnswerAvailable = ObservableField(false)
val selectedItemText = ObservableField("Please select atleast one choice.")
Copy link
Member

Choose a reason for hiding this comment

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

We need to use string resources for these strings, instead, since they need to be translatable.

.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt
@@ -138,7 +140,9 @@ class SelectionInteractionViewModel private constructor(
// TODO(#3624): Add warning to user when they exceed the number of allowable selections or are under the minimum
// number required.
selectedItems += itemIndex
selectedItemText.set("You may select more choices")
Copy link
Member

Choose a reason for hiding this comment

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

Could this be logically moved into updateSelectionText to avoid needing to set the item text multiple times here? It may also help with robustness since it ensures that updateSelectionText always uses the most correct text.

@oppiabot
Copy link

oppiabot bot commented Aug 31, 2022

Hi @Akshatkamboj14, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 31, 2022
@Akshatkamboj14
Copy link
Member Author

Closing this PR because @seanlip suggested creating a new PR as per the discussion in the #3169 comment section.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning requested changes

@rt4914 rt4914 Awaiting requested review from rt4914

Assignees

@Akshatkamboj14 Akshatkamboj14

Labels
stale Corresponds to items that haven't seen a recent update and may be automatically closed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ItemSelectionInteraction text change on selection
3 participants
@Akshatkamboj14 @rt4914 @BenHenning