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 part of #2616: Admin Auth Screen[a11y] #2649

Merged
merged 4 commits into from
Feb 18, 2021

Conversation

Arjupta
Copy link
Contributor

@Arjupta Arjupta commented Feb 7, 2021

Explanation

Fixes part of #2616 ie. the TalkBack response for disabled submit button by using enabled property

When disabled When Enabled
WhatsApp Image 2021-02-08 at 3 57 21 PM Screenshot_2021-02-07-13-21-09-71_943a62cb4c6fb83e010e1c2e82766a17

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

Fixes the TalkBack response for disabled submit button by adding content
description
@Arjupta
Copy link
Contributor Author

Arjupta commented Feb 7, 2021

@rt4914 and @FareesHussain can you find why tests are failing.

@FareesHussain
Copy link
Contributor

Tests needs to be re-run as these test pass locally
image

image

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.

@Arjupta This solution is not correct.
Couple of points:

  1. SUBMIT, Button, disabled is not same as Disabled Submit, Button. In first case the disabled is not typed anywhere it is called by Talkback automatically. Similar to the case where Button is not typed anywhere but the Talkback automatically outputs that.
  2. To fix this revert these changes and simply introduce: android:enabled="@{viewModel.isSubmitButtonActive}" and test the app again on talkback.

@rt4914
Copy link
Contributor

rt4914 commented Feb 8, 2021

Also add Fixes part of #bugnumber in description too.

@rt4914 rt4914 assigned Arjupta and unassigned rt4914 Feb 8, 2021
@rt4914
Copy link
Contributor

rt4914 commented Feb 8, 2021

@rt4914 and @FareesHussain can you find why tests are failing.

There are couple of flaky tests nothing that your PR is related to so don't worry about this.

@Arjupta
Copy link
Contributor Author

Arjupta commented Feb 8, 2021

@Arjupta This solution is not correct.
Couple of points:

  1. SUBMIT, Button, disabled is not same as Disabled Submit, Button. In first case the disabled is not typed anywhere it is called by Talkback automatically. Similar to the case where Button is not typed anywhere but the Talkback automatically outputs that.
  2. To fix this revert these changes and simply introduce: android:enabled="@{viewModel.isSubmitButtonActive}" and test the app again on talkback.

Okay! doing the changes

@Arjupta Arjupta assigned rt4914 and unassigned Arjupta Feb 8, 2021
@Arjupta
Copy link
Contributor Author

Arjupta commented Feb 8, 2021

Also add Fixes part of #bugnumber in description too.

Updated the screenshot too.

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, thanks

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.

Meant to approve

@rt4914 rt4914 enabled auto-merge (squash) February 8, 2021 14:26
@rt4914 rt4914 removed their assignment Feb 8, 2021
@Arjupta
Copy link
Contributor Author

Arjupta commented Feb 10, 2021

@rt4914 this PR is not merged, github bug i guess

@rt4914 rt4914 disabled auto-merge February 10, 2021 21:15
@rt4914 rt4914 closed this Feb 10, 2021
@rt4914 rt4914 reopened this Feb 10, 2021
@rt4914 rt4914 enabled auto-merge (squash) February 10, 2021 21:16
@rt4914
Copy link
Contributor

rt4914 commented Feb 17, 2021

@Arjupta Once you update the branch it will get merged

@rt4914 rt4914 merged commit d4a945f into oppia:develop Feb 18, 2021
@Arjupta Arjupta deleted the a11y-admin-add-pin branch May 20, 2021 21:24
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.

None yet

3 participants