-
Notifications
You must be signed in to change notification settings - Fork 509
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 #3131: Add Profile toggle radio buttons by clicking on the complete item #3191
Conversation
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.
@rt4914 Left a comment, PTAL
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.
LGTM, 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.
@rt4914 the text output that we see here -
"Allow Download Access, Enable the ... Administrator Pin, not checked, OFF, Switch"
Is this bevaiour normal to repeat the mention of switch-state? ( ie not checked and OFF)
@Arjupta yes. I have checked switches across multiple apps and I have found three types of behaviour: |
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.
LGTM
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
Left few thoughts/questions.
app/src/main/java/org/oppia/android/app/profile/AddProfileActivityPresenter.kt
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/profile/AddProfileActivityTest.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.
Sounds good.
Could you add espresso result screenshot?
…lete-toggle-click
…lete-toggle-click
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.
LGTM, Thanks @rt4914
Explanation
Fixes #3131
This PR makes the entire
Switch + 2 Texts
clickable in add profile. Also the changes have been made to make it correctly accessible too which is shown in below screenshot:Checklist