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 #2325: Changes in Cellular dialog[a11y] #2519

Merged
merged 2 commits into from
Jan 21, 2021

Conversation

Arjupta
Copy link
Contributor

@Arjupta Arjupta commented Jan 19, 2021

Explanation

Fixes Issue 2 of #2325 by increasing the clickable checkbox minimum height to be 48dp

Before Changes After Changes Test Result
Screenshot_1611150898 Screenshot_1611150837 Screenshot_1611150405

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 Issue 2 of oppia#2325 by increasing the clickable checkbox minimum height to be 28dp
@Arjupta Arjupta requested a review from rt4914 as a code owner January 19, 2021 13:19
@Arjupta Arjupta marked this pull request as draft January 19, 2021 13:24
@rt4914
Copy link
Contributor

rt4914 commented Jan 19, 2021

@Arjupta In PRs related to a11y, always post following things:

  1. Before screenshot of UI
  2. After screenshot of UI.
  3. A11y scanner result.

@rt4914
Copy link
Contributor

rt4914 commented Jan 19, 2021

Update title to Fixes part of #2325 as this PR is not solving the entire issue.

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.

Overall good. Just need to do the changes mentioned above and remove from draft.

@Arjupta Arjupta changed the title Fix #2325: Changes in Cellular dialog[a11y] Fixes part of #2325: Changes in Cellular dialog[a11y] Jan 20, 2021
@Arjupta Arjupta marked this pull request as ready for review January 20, 2021 13:58
@Arjupta
Copy link
Contributor Author

Arjupta commented Jan 20, 2021

@Arjupta In PRs related to a11y, always post following things:

  1. Before screenshot of UI
  2. After screenshot of UI.
  3. A11y scanner result.

@rt4914 I have done required changes but as you can see that there is a slight variation in the height of dialog because of the changes. This is not actually breaking the UI but still a bit different from the previous one

@rt4914
Copy link
Contributor

rt4914 commented Jan 21, 2021

@Arjupta In PRs related to a11y, always post following things:

  1. Before screenshot of UI
  2. After screenshot of UI.
  3. A11y scanner result.

@rt4914 I have done required changes but as you can see that there is a slight variation in the height of dialog because of the changes. This is not actually breaking the UI but still a bit different from the previous one

@Arjupta That's not an issue because for all AlertDialog we are not trying to copy the UI to pixel perfect instead we reply on Android Native UI to make sure it looks good. So don't worry about this nit change.

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.

@rt4914
Copy link
Contributor

rt4914 commented Jan 21, 2021

Also update your branch with latest develop. @Arjupta

@rt4914 rt4914 enabled auto-merge (squash) January 21, 2021 11:11
@Arjupta
Copy link
Contributor Author

Arjupta commented Jan 21, 2021

Also update your branch with latest develop. @Arjupta

done

@rt4914 rt4914 merged commit 3c32562 into oppia:develop Jan 21, 2021
@Arjupta Arjupta deleted the a11y-audio-player-related-views branch May 20, 2021 21:26
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.

2 participants