-
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
Fixes part of #3362: Fix test cases for AccessibilityChecks #3372
Conversation
@@ -46,6 +46,7 @@ | |||
android:focusableInTouchMode="true" | |||
android:fontFamily="sans-serif" | |||
android:marqueeRepeatLimit="1" | |||
android:minHeight="48dp" |
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.
As this is clickable, it should be 48dp
min height.
@@ -45,6 +45,7 @@ | |||
android:focusableInTouchMode="true" | |||
android:fontFamily="sans-serif" | |||
android:marqueeRepeatLimit="1" | |||
android:minHeight="48dp" |
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.
As this is clickable, it should be 48dp
min height.
@@ -68,6 +48,16 @@ | |||
android:text="@{htmlContent}" | |||
android:textColor="@color/oppiaPrimaryText" | |||
android:textSize="16sp" | |||
android:visibility="@{htmlContent.length() > 0 ? View.VISIBLE : View.GONE, default=gone}" /> | |||
android:visibility="@{htmlContent.length() > 0 ? View.VISIBLE : View.GONE, default=gone}" |
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.
Content-item can have internal-links
which are clickable and therefore the min-height should be 48dp
. Now consider a case where the text is only of 1 line
, in that case the min-height is less than 48dp
and therefore I have shifted the padding of FrameLayout
to TextView
and also I have increased the padding-top/bottom from 12dp to 16dp in dimens.xml
files too.
Same concept applies for FeedbackItem
too.
@@ -68,6 +48,16 @@ | |||
android:text="@{htmlContent}" | |||
android:textColor="@color/oppiaPrimaryText" | |||
android:textSize="16sp" | |||
android:visibility="@{htmlContent.length() > 0 ? View.VISIBLE : View.GONE, default=gone}" /> |
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.
FeedbackItem
can have internal-links
which are clickable and therefore the min-height should be 48dp
. Now consider a case where the text is only of 1 line
, in that case the min-height is less than 48dp
and therefore I have shifted the padding of FrameLayout
to TextView
and also I have increased the padding-top/bottom from 12dp to 16dp in dimens.xml
files too.
Same concept applies for ContentItem
too.
@@ -44,6 +44,7 @@ | |||
android:layout_height="wrap_content" | |||
android:layout_marginEnd="20dp" | |||
android:layout_weight="1" | |||
android:minHeight="48dp" |
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.
As this is clickable, it should be 48dp
min height.
@@ -40,6 +40,7 @@ | |||
android:focusableInTouchMode="true" | |||
android:fontFamily="sans-serif" | |||
android:marqueeRepeatLimit="1" | |||
android:minHeight="48dp" |
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.
As this is clickable, it should be 48dp
min height.
@@ -45,6 +45,7 @@ | |||
android:focusableInTouchMode="true" | |||
android:fontFamily="sans-serif" | |||
android:marqueeRepeatLimit="1" | |||
android:minHeight="48dp" |
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.
As this is clickable, it should be 48dp min height.
@@ -252,7 +252,6 @@ class ExplorationActivityTest { | |||
} | |||
|
|||
@Test | |||
@DisableAccessibilityChecks // TODO(#3251): Enable AccessibilityChecks |
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.
@@ -288,7 +288,6 @@ class StateFragmentTest { | |||
} | |||
|
|||
@Test | |||
@DisableAccessibilityChecks // TODO(#3251): Enable AccessibilityChecks |
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.
The changes in toolbar and content/feedback item fixes 36
test cases in this. 12
test cases are still pending which will be fixed later.
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.
@@ -119,7 +118,6 @@ class RevisionCardFragmentTest { | |||
} | |||
|
|||
@Test | |||
@DisableAccessibilityChecks // TODO(#3251): Enable AccessibilityChecks |
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.
Fixed because we mentioned minHeight 48dp
in toolbar text.
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.
Re running gradel task |
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, waiting for gradel task to pass
Unassigning @anandwana001 since they have already approved the PR. |
Explanation
Fixes part of #3362: Fix test cases for AccessibilityChecks
This PR fixes some of the AccessibilityChecks failures across multiple test files.
Please check comments for better understanding.
Checklist