-
Notifications
You must be signed in to change notification settings - Fork 506
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 #2628: Reading Text Size[A11y] #2929
Conversation
Fixes oppia#2628 based on the mock provided here oppia#2628 (comment)
@rt4914 and @mschanteltc we need to have content description for the radio buttons also as they give this output with the TalkBack
|
@Arjupta any update on this? |
Pardon, I will replace these tests soon. |
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.
Thanks @Arjupta
left few changes
app/src/main/java/org/oppia/android/app/options/ReadingTextSizeSelectionViewModel.kt
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/test/java/org/oppia/android/app/testing/options/ReadingTextSizeFragmentTest.kt
Outdated
Show resolved
Hide resolved
Looking for @rt4914 thoughts on this. |
@Arjupta and @anandwana001 Sent an email to both of you. |
private const val SMALL_TEXT_SIZE = 0 | ||
private const val MEDIUM_TEXT_SIZE = 5 | ||
private const val LARGE_TEXT_SIZE = 10 | ||
private const val SMALL_TEXT_SIZE_SCALE = 0.8f |
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.
Add following comments:
- These duplicates are intentional to make sure that that the production values are not changed by mistake.
- TODO Intoduce screenshot-testing framework #1815: Remove these duplicate values once Screenshot tests are implemented.
@Arjupta PTAL
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.
@rt4914 LGTM. Adding them
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.
Is this TODO format is correct?
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.
Format can be confirmed from code. I just mentioned it as points.
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.
@Arjupta Format these comments correctly.
// These duplicates are intentional to make sure that that the production values are not changed
// by mistake.
// TODO(#59): 1815: Remove these duplicate values once Screenshot tests are implemented.
The above comment was for understanding not in comments format.
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, left one comment regarding TODO format.
private const val SMALL_TEXT_SIZE = 0 | ||
private const val MEDIUM_TEXT_SIZE = 5 | ||
private const val LARGE_TEXT_SIZE = 10 | ||
private const val SMALL_TEXT_SIZE_SCALE = 0.8f |
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.
Is this TODO format is correct?
Assigning @BenHenning for code owner reviews. Thanks! |
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.
@Arjupta Blocked on the comment formatting in tests and bazel check failure.
@Arjupta tests are still failing. |
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, thanks.
Explanation
Fixes #2628 based on the mock provided here
Tests passing-
Checklist