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.

Sign up for GitHub

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

Jump to bottom

Fixes part of #2258: Changes in Add Profile Activity[a11y] #2456

Merged
merged 4 commits into from
Feb 3, 2021
Merged

Fixes part of #2258: Changes in Add Profile Activity[a11y] #2456

merged 4 commits into from
Feb 3, 2021

Conversation

Arjupta
Copy link
Contributor

@Arjupta Arjupta commented Jan 11, 2021 •
edited
Loading

Explanation

Fixes part of #2258,
( Issue 3 ) Changed the button to disabled according to the state and TextColor as per Chantel's suggestion

Before After Tests
Screenshot_1611489937 Screenshot_1611490274 Screenshot_1611490266

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.

@Arjupta
Fix oppia#2258: Changes in Add Profile Activity[a11y]
b4034d2 
Fixes Issue 3 of oppia#2258, Changed the button to diabled according to the
state and TextColor as per Chantel's suggesstion
@Arjupta
Copy link
Contributor Author

Arjupta commented Jan 11, 2021

@rt4914 Is it failing because of the that randomly failing test?

rt4914
rt4914 reviewed Jan 11, 2021
View reviewed changes
app/src/main/res/layout/add_profile_activity.xml Outdated
@@ -205,7 +205,7 @@
android:layout_height="wrap_content"
android:fontFamily="sans-serif"
android:text="@string/add_profile_allow_download_sub"
android:textColor="@color/light_grey"
android:textColor="#555555"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define this color in colors.xml file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

app/src/main/res/layout/add_profile_activity.xml Outdated
@@ -194,7 +194,7 @@
android:layout_height="wrap_content"
android:fontFamily="sans-serif"
android:text="@string/add_profile_allow_download_heading"
android:textColor="@{viewModel.validPin ? @color/oppiaPrimaryTextDark : @color/grey}"
android:textColor="#333333"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Define this color in colors.xml file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Arjupta Arjupta changed the title Fix #2258: Changes in Add Profile Activity[a11y] Fixes part of #2258: Changes in Add Profile Activity[a11y] Jan 23, 2021
@Arjupta Arjupta marked this pull request as ready for review January 23, 2021 16:29
rt4914
rt4914 suggested changes Jan 23, 2021
View reviewed changes
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 Please add before and after screenshot along with the scanner result screenshot (even if it is successful).

@rt4914 rt4914 assigned Arjupta and unassigned rt4914 Jan 23, 2021
@Arjupta
Copy link
Contributor Author

Arjupta commented Jan 24, 2021

@rt4914 In this PR I changed the colors related to Allow Download View , and it also did got solved, but the Input Layouts are now showing the contrast ratio error too (which on previous scans on real device didn't showed up).
Screenshot_1611490278 Screenshot_1611494560

Can you verify at your end?

@Arjupta Arjupta assigned rt4914 and unassigned Arjupta Jan 24, 2021
rt4914
rt4914 suggested changes Jan 25, 2021
View reviewed changes
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 PTAL. Thanks.

app/src/main/res/layout-land/add_profile_activity.xml Outdated
@@ -196,7 +196,7 @@
android:layout_height="wrap_content"
android:fontFamily="sans-serif"
android:text="@string/add_profile_allow_download_heading"
android:textColor="@{viewModel.validPin ? @color/oppiaPrimaryTextDark : @color/grey}"
android:textColor="@color/accessible_grey"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this PR we have to replace grey with accessible_grey .

Example:
android:textColor="@{viewModel.validPin ? @color/oppiaPrimaryTextDark : @color/accessible_grey}"

Can you check if the scanner passes for both these cases of color code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original PR After Suggested Changes
Screenshot_1611641944 Screenshot_1611642079
Screenshot_1611642012 Screenshot_1611642131

Scans are passing for both but Chantel's commented to keep the text color to be this for the entire time, but if this passes then it is better I guess , WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The earlier implementation was actually more correct because when the checkbox is selected the text becomes highlighted which gives better look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant to say until the Text Input Layout have some content them, the textviews seemed to be inactive. Because checkbox controls the visibility of all the content below it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right, the correctness of pin controls the color of switch related texts.

@rt4914 rt4914 assigned Arjupta and unassigned rt4914 Jan 25, 2021
@Arjupta Arjupta assigned rt4914 and unassigned Arjupta Jan 26, 2021
@rt4914 rt4914 assigned Arjupta and unassigned rt4914 Jan 27, 2021
@Luffy18346
Copy link
Contributor

@Arjupta PTAL

@Arjupta Arjupta assigned rt4914 and unassigned Arjupta Feb 2, 2021
rt4914
rt4914 approved these changes Feb 3, 2021
View reviewed changes
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 2ed1870 into oppia:develop Feb 3, 2021
@Arjupta Arjupta deleted the a11y-add-profile-activity branch May 20, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Arjupta @Luffy18346 @rt4914