-
Notifications
You must be signed in to change notification settings - Fork 499
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
Fix part of #3602: Add label for Walk Through Activity #3983
Fix part of #3602: Add label for Walk Through Activity #3983
Conversation
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 @priyatanu! I left one high-level comment. However, beyond that:
- The PR title should be "Fix part of [A11y] Add label for all non-test activities #3602" since this isn't addressing everything covered by that issue
- You should add a description in the explanation section of the PR to explain what this PR is doing
- The "For UI-specific PRs only" portion of the PR description needs to be filled in since this PR is changing a user behavior
Please update these & at-mention me once they've been addressed.
app/src/main/AndroidManifest.xml
Outdated
@@ -215,6 +215,7 @@ | |||
android:theme="@style/OppiaThemeWithoutActionBar" /> | |||
<activity | |||
android:name=".app.walkthrough.WalkthroughActivity" | |||
android:label="@string/walk_through_activity_title" |
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.
We also need to make sure that there's a test to verify this. See https://github.com/oppia/oppia-android/pull/3227/files as a reference PR for all the parts needed to add a new accessibility label.
Hi @priyatanu, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Thank you. I will look into this |
FYI I'm listed as a codeowner for this PR & I'll be unavailable to perform code reviews over the next 2 weeks--thanks for your patience. |
@Arjupta Can you please review this PR first? Thanks. |
@priyatanu Thanks for the PR. Please assign PRs to @Arjupta first has he your mentor that way its easier to review PRs and communicate effectively. |
Thank you |
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.
@priyatanu PTAL.
Also please add screenshot in PR description showing that test is is passing on Espresso.
app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityLabelTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityLabelTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityLabelTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityLabelTest.kt
Outdated
Show resolved
Hide resolved
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.
@priyatanu one static check is failing because we have previously kept the WalkthroughActivity
exempted from failing accessiblity issues but as you have resolved them now so it is unnecessary for the file to be exempted further, hence you need to remove the name of WalkthroughActivity
from scripts/assets/accessibility_label_exemptions.textproto
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.
@priyatanu PTAL thanks.
Also, please mention Fix part of #3602: Add label for Walk Through Activity
in your PR description also.
app/src/sharedTest/java/org/oppia/android/app/walkthrough/WalkthroughActivityTest.kt
Outdated
Show resolved
Hide resolved
Hi @priyatanu, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Hi. As of today, some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 4 January 2021. |
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. @rt4914 PTAL
Assigning @rt4914 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.
LGTM, 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.
Thanks @priyatanu! Sorry for the late review--this LGTM!
Explanation
Fix part of #3602: Add label for Walk Through Activity
Essential Checklist