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 #2326: Changes in Views inside Practice Tab [A11y] #2525

Merged
merged 4 commits into from
Jan 21, 2021

Conversation

Mehul-Bisht
Copy link
Contributor

@Mehul-Bisht Mehul-Bisht commented Jan 20, 2021

Explanation

Fixes part of #2326: Changes in Views inside Practice Tab [A11y]
Fixed issue with checkbox accessibility for all layouts

Portrait Mode

Before After
portrait_before portrait

Landscape Mode

Status

Before changes

land_before

After Changes

land_top_after
land_bottom

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.

@Mehul-Bisht Mehul-Bisht requested a review from rt4914 as a code owner January 20, 2021 14:01
@Mehul-Bisht
Copy link
Contributor Author

@rt4914 @FareesHussain please review my PR and notify about any improvements related to checkboxes

@Arjupta
Copy link
Contributor

Arjupta commented Jan 20, 2021

@Mehul-Bisht the topic_practice_header_view.xml does not need to be changed as the START button resides in topic_practice_footer_view.xml

@Mehul-Bisht
Copy link
Contributor Author

@Mehul-Bisht the topic_practice_header_view.xml does not need to be changed as the START button resides in topic_practice_footer_view.xml

Yes I know, but the reason why I reduced padding at the bottom for the header was that I made changes to padding of the topic_practice_subtopic. So now the subtopic items have more padding at the top so the header doesn't need as much padding on the bottom as it had before. May I attach a screenshot to show the changes?

@Arjupta
Copy link
Contributor

Arjupta commented Jan 20, 2021

@Mehul-Bisht the topic_practice_header_view.xml does not need to be changed as the START button resides in topic_practice_footer_view.xml

Yes I know, but the reason why I reduced padding at the bottom for the header was that I made changes to padding of the topic_practice_subtopic. So now the subtopic items have more padding at the top so the header doesn't need as much padding on the bottom as it had before. May I attach a screenshot to show the changes?

Yes, also attach the screenshots for before the changes and of passing scans

@Mehul-Bisht
Copy link
Contributor Author

Before changes screenshots are already shown here: #2326

After changes the scans for:

Portrait Mode:

portrait

Landscape Mode:

land_bottom
land_top

@Arjupta
Copy link
Contributor

Arjupta commented Jan 20, 2021

@Mehul-Bisht you have done a good work . Even if we have solved the issue this way, but we don't want the UI to change drastically.

Before After
Screenshot_1611155272 Screenshot_1611155350

You must try keeping the topic_practice_header_view to be unchanged along with aligning the checkboxes to be start|top instead of start|bottom

@Mehul-Bisht
Copy link
Contributor Author

Ok I will update it soon.

@Mehul-Bisht
Copy link
Contributor Author

@Arjupta I have now made the specified changes in all practice tab layout files. Kindly review.

@FareesHussain FareesHussain assigned Arjupta and unassigned Mehul-Bisht Jan 20, 2021
@Arjupta
Copy link
Contributor

Arjupta commented Jan 21, 2021

@Arjupta I have now made the specified changes in all practice tab layout files. Kindly review.

Thanks @Mehul-Bisht , nit changes

  • Edit the title to be Fixes Part of #2326 as you have not worked on complete Issue
  • Attach some Screenshots in the PR description like below

@rt4914 Rest everything LGTM.

Before After Scan
Screenshot_1611155272 Screenshot_1611205113 Screenshot_1611205191

@Arjupta Arjupta assigned rt4914 and Mehul-Bisht and unassigned Arjupta Jan 21, 2021
@Arjupta
Copy link
Contributor

Arjupta commented Jan 21, 2021

@rt4914 while testing this for landscape mode, I encountered another issue

Screenshot_1611207456 Screenshot_1611207443

I think this is because of the drop-shadow view, I tried to set it not important for accessibility but still the checks failed at this giving wrong indication about contrast issue in checkbox.

@Mehul-Bisht Mehul-Bisht changed the title Fix #2326: Changes in Views inside Practice Tab [A11y] Fixes Part of #2326: Changes in Views inside Practice Tab [A11y] Jan 21, 2021
@rt4914
Copy link
Contributor

rt4914 commented Jan 21, 2021

@rt4914 while testing this for landscape mode, I encountered another issue

Screenshot_1611207456 Screenshot_1611207443
I think this is because of the drop-shadow view, I tried to set it not important for accessibility but still the checks failed at this giving wrong indication about contrast issue in checkbox.

@Arjupta File an issue mentioning how to reproduce this and we will have a look. Thanks.

@rt4914
Copy link
Contributor

rt4914 commented Jan 21, 2021

@Mehul-Bisht Update your PR description to
Fixes part of #2326: Changes in Views inside Practice Tab [A11y]
Otherwise the entire issue will get closed.

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.

Really nice work @Mehul-Bisht . Suggested one nit change.

app/src/main/res/layout/topic_practice_subtopic.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Jan 21, 2021
@Mehul-Bisht
Copy link
Contributor Author

@rt4914 @Arjupta I have made the changes to the PR title, description and minHeight attribute now, please review again.

@Mehul-Bisht Mehul-Bisht assigned Arjupta and unassigned Mehul-Bisht Jan 21, 2021
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 rt4914 merged commit 1144075 into oppia:develop Jan 21, 2021
@Arjupta
Copy link
Contributor

Arjupta commented Jan 24, 2021

@Mehul-Bisht and @rt4914 are you facing this issue on a Tablet ?

Screenshot_1611506427 Screenshot_1611506115

@Mehul-Bisht
Copy link
Contributor Author

@Arjupta I could not test it with a tablet since my device doesn't run emulators that well. I tried to select an appropriate value for the tablet layout.

@Arjupta
Copy link
Contributor

Arjupta commented Jan 25, 2021

@Arjupta I could not test it with a tablet since my device doesn't run emulators that well. I tried to select an appropriate value for the tablet layout.

It's okay . For future PR mention the tests you have done on your side.

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.

3 participants