Fix #3169: ItemSelectionInteraction text change on selection #3814

darkmat13r · 2021-09-18T03:55:00Z

Explanation

Fix #3169 : ItemSelectionInteraction text change on selection

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

Before

After

…nteraction_text_change

darkmat13r · 2021-09-18T03:55:23Z

@rt4914 PTAL

rt4914

@darkmat13r Thanks a lot. Just need to add one more test case otherwise LGTM

app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt

…nteraction_text_change

rt4914

LGTM, thanks.

@BenHenning Any idea why bazel tests are failing?

BenHenning

Thanks @darkmat13r! Just had a few comments--PTAL.

BenHenning · 2021-09-26T04:14:40Z

app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt

+  }
+
+  @Test
+  fun testStateFragment_interactions_land_singleItemSelected_selectionTextIsNotDisplayed() {


Can you add tests to demonstrate that the selection text retains its showing/not showing behavior after performing a rotation & activity recreation?

@BenHenning Yes. I will add the test case.

@BenHenning I have remove the landscape test cases as these doesn't make sense because currently when you select some answers and rotate the screen all values gets lost. So right now we can't add those test cases related to orientation change or activity recreation.
Follow are the removed test cases for landscape orientation:
testStateFragment_interactions_land_noItemSelected_selectionTextIsDisplayed
testStateFragment_interactions_land_singleItemSelected_selectionTextIsNotDisplayed
testStateFragment_interactions_land_multipleItemSelected_selectionTextIsNotDisplayed
testStateFragment_interactions_changeConfiguration_selectUnselectCheckbox_selectionTextIsDisplayedCorrectly

@BenHenning PTAL

Ah, yes that's related to #1594. Fair point--I think omitting these tests are fine. Instead, can you add a TODO, such as:

// TODO(#1594): Add tests to ensure the selection text is properly showing/not showing after a configuration change.

app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt

.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt

BenHenning · 2021-09-26T04:17:49Z

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

@@ -46,7 +46,7 @@
      android:textColor="@color/oppiaPrimaryText"
      android:textSize="14sp"
      android:textStyle="italic"
-      android:visibility="@{viewModel.getSelectionItemInputType() == SelectionItemInputType.RADIO_BUTTONS ? View.GONE : View. VISIBLE}" />
+      android:visibility="@{viewModel.getSelectionItemInputType() == SelectionItemInputType.RADIO_BUTTONS || viewModel.selectedItemsCount > 0 ? View.GONE : View. VISIBLE}" />


Given the OR here, do we already have tests verifying that the text is always showing for radio buttons for both unselected & selected states to contrast with the checkbox behavior?

@BenHenning No, I will add them.

@BenHenning For the radio buttons the text will always be hidden. I am not sure we need tests for this.

@BenHenning I have added test case for the radio buttons.

@BenHenning PTAL

I think you also need a test for state 2 (which is MultipleChoiceInput) to ensure that the text view is correctly arranged for that interaction, as well.

@BenHenning state 2 is Fraction input option. State 3 and state 4 are multiple choice input. I have already added the test case for state 4.

@Test fun testStateFragment_interactions_radioItemSelection_selectionTextIsNotDisplayed() { launchForExploration(TEST_EXPLORATION_ID_2, shouldSavePartialProgress = false).use { startPlayingExploration() playThroughPrototypeState1() playThroughPrototypeState2() playThroughPrototypeState3() // Verify that the user is now on the fourth state. verifyViewTypeIsPresent(SELECTION_INTERACTION) onView(withId(R.id.selection_interaction_textview)).check(matches(not(isDisplayed()))) } }

//Added these additional test cases

testStateFragment_interactions_singleRadioItemSelected_selectionTextIsNotDisplayed testStateFragment_interactions_multipleRadioItemSelected_selectionTextIsNotDisplayed testStateFragment_interactions_correctRadioItemSelected_selectionTextIsNotDisplayed

…nteraction_text_change

oppiabot · 2021-10-10T13:59:10Z

Hi @darkmat13r, 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!

…eraction_text_change

oppiabot · 2021-10-28T06:04:40Z

Hi @darkmat13r, 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!

…nteraction_text_change

oppiabot · 2021-11-05T05:09:17Z

Hi @darkmat13r, 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!

…nteraction_text_change

oppiabot · 2021-11-14T05:26:45Z

Hi @darkmat13r, 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!

rt4914 · 2021-11-18T20:33:54Z

@darkmat13r Can you please reply to all the open-comments, that way based on reply Ben can decide if that comment can be resolved or not. Thanks

darkmat13r · 2021-11-19T04:15:07Z

@darkmat13r Can you please reply to all the open-comments, that way based on reply Ben can decide if that comment can be resolved or not. Thanks

@BenHenning @rt4914 All the comments are resolved but I am not able to run the test cases. Some test cases are failing so I need to fix them.

BenHenning · 2021-11-20T00:55:20Z

FYI I'm listed as a codeowner for this PR & I'll be unavailable to perform code reviews over the next 2 weeks--thanks for your patience.

BenHenning · 2021-11-20T06:58:35Z

@darkmat13r Can you please reply to all the open-comments, that way based on reply Ben can decide if that comment can be resolved or not. Thanks

@BenHenning @rt4914 All the comments are resolved but I am not able to run the test cases. Some test cases are failing so I need to fix them.

@darkmat13r why aren't you able to run the test cases? It'd be really helpful to get more context. Please see https://github.com/oppia/oppia-android/wiki/Frequent-Errors-and-Solutions#facing-error-while-debugging-code which includes tips on debugging, and instructions for creating a debug doc that you can share with folks to make it easier for them to help you.

darkmat13r · 2021-11-23T13:02:57Z

@BenHenning Not able to run the test cases on android studio artic fox version. The solution i mention to work on Java Idea community version also not working . I will provide more details on the error.
#3783

oppiabot · 2021-12-03T18:04:30Z

Hi @darkmat13r, 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!

darkmat13r added 2 commits September 16, 2021 10:55

hide selection interaction text when more than one checkbox selected

2b1b777

Merge remote-tracking branch 'upstream/develop' into item_selection_i…

f978c06

…nteraction_text_change

darkmat13r requested review from BenHenning and rt4914 as code owners September 18, 2021 03:55

oppiabot bot assigned rt4914 Sep 18, 2021

rt4914 suggested changes Sep 21, 2021

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt Show resolved Hide resolved

rt4914 assigned darkmat13r and unassigned rt4914 Sep 21, 2021

darkmat13r added 2 commits September 22, 2021 10:36

add test case for check and uncheck selection

01aa5e8

Merge remote-tracking branch 'upstream/develop' into item_selection_i…

ec6216a

…nteraction_text_change

rt4914 assigned rt4914 and unassigned darkmat13r Sep 24, 2021

rt4914 approved these changes Sep 24, 2021

View reviewed changes

rt4914 assigned BenHenning and unassigned rt4914 Sep 24, 2021

BenHenning changed the title ~~Fix ##3169 : ItemSelectionInteraction text change on selection~~ Fix #3169: ItemSelectionInteraction text change on selection Sep 26, 2021

BenHenning requested changes Sep 26, 2021

View reviewed changes

BenHenning assigned darkmat13r and unassigned BenHenning Sep 26, 2021

darkmat13r added 7 commits September 29, 2021 20:00

remove redundant test

2bd9a64

Added Text visibility test when interaction type is radio button

6bdd067

Merge remote-tracking branch 'upstream/develop' into item_selection_i…

404d83d

…nteraction_text_change

Doc for selected item count

c022274

Fix lint errors

0d8b8a3

Update selectedItemsCount doc

ec409b4

remove landscape test cases

f9707c1

oppiabot bot added stale Corresponds to items that haven't seen a recent update and may be automatically closed. and removed stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Oct 10, 2021

BenHenning assigned darkmat13r Oct 21, 2021

Merge remote-tracking branch 'origin/develop' into item_selection_int…

db054c7

…eraction_text_change

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 28, 2021

darkmat13r added 3 commits October 29, 2021 09:34

Merge remote-tracking branch 'upstream/develop' into item_selection_i…

0e2321a

…nteraction_text_change

Update comments

c326fe8

Update comments

52c2b48

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Oct 29, 2021

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 5, 2021

darkmat13r added 2 commits November 7, 2021 09:36

Merge remote-tracking branch 'upstream/develop' into item_selection_i…

19771f3

…nteraction_text_change

Test cases for state 4 multiple input selection

c4e33c0

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 7, 2021

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 14, 2021

rt4914 self-assigned this Nov 17, 2021

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Nov 17, 2021

rt4914 removed their assignment Nov 18, 2021

BenHenning self-assigned this Nov 20, 2021

BenHenning removed their assignment Nov 20, 2021

rt4914 assigned rt4914 and unassigned rt4914 Nov 24, 2021

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 3, 2021

oppiabot bot closed this Dec 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3169: ItemSelectionInteraction text change on selection #3814

Fix #3169: ItemSelectionInteraction text change on selection #3814

darkmat13r commented Sep 18, 2021 •

edited by BenHenning

Loading

darkmat13r commented Sep 18, 2021

rt4914 left a comment

rt4914 left a comment

BenHenning left a comment

BenHenning Sep 26, 2021

darkmat13r Sep 29, 2021

darkmat13r Oct 3, 2021

darkmat13r Oct 11, 2021

BenHenning Oct 21, 2021

BenHenning Sep 26, 2021

darkmat13r Sep 27, 2021

darkmat13r Sep 29, 2021

darkmat13r Oct 3, 2021

darkmat13r Oct 11, 2021

BenHenning Oct 21, 2021

darkmat13r Nov 7, 2021

oppiabot bot commented Oct 10, 2021

oppiabot bot commented Oct 28, 2021

oppiabot bot commented Nov 5, 2021

oppiabot bot commented Nov 14, 2021

rt4914 commented Nov 18, 2021

darkmat13r commented Nov 19, 2021

BenHenning commented Nov 20, 2021

BenHenning commented Nov 20, 2021

darkmat13r commented Nov 23, 2021 •

edited

Loading

oppiabot bot commented Dec 3, 2021

Fix #3169: ItemSelectionInteraction text change on selection #3814

Fix #3169: ItemSelectionInteraction text change on selection #3814

Conversation

darkmat13r commented Sep 18, 2021 • edited by BenHenning Loading

Explanation

Essential Checklist

For UI-specific PRs only

Before

After

darkmat13r commented Sep 18, 2021

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 10, 2021

oppiabot bot commented Oct 28, 2021

oppiabot bot commented Nov 5, 2021

oppiabot bot commented Nov 14, 2021

rt4914 commented Nov 18, 2021

darkmat13r commented Nov 19, 2021

BenHenning commented Nov 20, 2021

BenHenning commented Nov 20, 2021

darkmat13r commented Nov 23, 2021 • edited Loading

oppiabot bot commented Dec 3, 2021

darkmat13r commented Sep 18, 2021 •

edited by BenHenning

Loading

darkmat13r commented Nov 23, 2021 •

edited

Loading