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.

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

Fixes #3169: Item selection interaction text change and checkboxes should be disabled on selection #4777

Conversation

Akshatkamboj14
Copy link
Member

@Akshatkamboj14 Akshatkamboj14 commented Nov 30, 2022

Explanation

Fixes #3169
Fixes #3624

This PR is doing 2 things -

  1. It's updating the textView based on the selection of checkboxes by the user, If the user has not selected any checkbox then the Text will be Please select all correct choices., If the user has selected less than maxSelection then the text will be You may select more choices. and if the user has selected maxSelection then the text will be No more than $maxSelection choices may be selected.

  2. If the user has selected maximum checkboxes that can be selected, then the non selected checkboxes will be disbaled, and if the user unselects the checkbox or checkboxes, then all checkboxes will get enabled.

This is the link to the repository for the comparison of the StateFragmentTest.kt, to show that the same tests are failing on both branches(i.e. my feature branch and my develop branch)
https://github.com/Akshatkamboj14/Test-Comparison-For-StateFragmentTest.kt-oppia-android

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
issue.webm

@BenHenning
Copy link
Member

As discussed during the meeting, there are some logic changes needed yet in the interaction view model. Please assign back once that's done @Akshatkamboj14.

… ItemSelectionInteraction-text-change-and-checkboxes-should-be-disabled-on-selection
@oppiabot
Copy link

oppiabot bot commented Dec 8, 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 Dec 8, 2022
… ItemSelectionInteraction-text-change-and-checkboxes-should-be-disabled-on-selection
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 10, 2022
@Akshatkamboj14 Akshatkamboj14 marked this pull request as draft December 10, 2022 12:58
@BenHenning BenHenning self-assigned this Dec 20, 2022
@BenHenning
Copy link
Member

Will need to follow up with a code suggestion tomorrow.

Ensure checkboxes have a disabled color.
@BenHenning
Copy link
Member

PTAL at Akshatkamboj14#1 & the checklist for fixes and remaining things to do that I noticed as part of creating that PR. Merging it will automatically update your branch & show up on this PR.

@BenHenning BenHenning removed their assignment Dec 23, 2022
@oppiabot
Copy link

oppiabot bot commented Dec 30, 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 Dec 30, 2022
Ensure items disable when hitting max.
… ItemSelectionInteraction-text-change-and-checkboxes-should-be-disabled-on-selection
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! The PR pretty much LGTM, just had one nit.

Besides that, I think you have one comment open from yourself that will need to be closed if it's no longer valid, and I think the PR needs to be updated with the latest develop. Please assign back once those are done, the last comment is addressed, and CI is passing (since then the PR should be ready to merge).

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. This LGTM!

@BenHenning
Copy link
Member

I believe this needs your review as well @rt4914. PTAL.

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Feb 28, 2023
@BenHenning
Copy link
Member

Also @Akshatkamboj14 please update your PR description to include a proper explanation of what this PR is doing, and to include screenshots and/or videos per the 'UI-specific only PRs' section to show the changed behaviors for both checkbox enabled/disabled states and the new item selection label text.

@Akshatkamboj14
Copy link
Member Author

Also @Akshatkamboj14 please update your PR description to include a proper explanation of what this PR is doing, and to include screenshots and/or videos per the 'UI-specific only PRs' section to show the changed behaviors for both checkbox enabled/disabled states and the new item selection label text.

Also @Akshatkamboj14 please update your PR description to include a proper explanation of what this PR is doing, and to include screenshots and/or videos per the 'UI-specific only PRs' section to show the changed behaviors for both checkbox enabled/disabled states and the new item selection label text.

Hey @BenHenning I have updated the PR Description and added supporting screen recording, Thanks.

@Akshatkamboj14 Akshatkamboj14 removed their assignment Feb 28, 2023
@Akshatkamboj14
Copy link
Member Author

Hey @rt4914 can you PTAL, its needs your review as well to get merged?

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.

LGTM

@rt4914 rt4914 merged commit c1ce5f8 into oppia:develop Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce warning for selections ItemSelectionInteraction text change on selection
3 participants