-
Notifications
You must be signed in to change notification settings - Fork 521
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 #3062: Label added to AdministratorControlsActivity. #3152
Fix #3062: Label added to AdministratorControlsActivity. #3152
Conversation
...edTest/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityTest.kt
Show resolved
Hide resolved
...edTest/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityTest.kt
Show resolved
Hide resolved
I unassigned myself and requested a review. |
@jkugsiya Could you confirm, if you have access to the Assignee section? |
|
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.
@jkugsiya Thanks.
...edTest/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityTest.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.
@jkugsiya One nit change.
Also, this PR contains changes related to formatting of the AndroidManifest
and strings
file which I think is correct change otherwise other contributors will face this same issue. But I will defer to @BenHenning about this.
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.
I've updated the fork's branch and that is reflecting the changes and showing that it is up-to-date. https://github.com/jkugsiya/oppia-android/tree/A11y-AdministratorControlsActivity But I don't know why the same is not reflecting in the PR. Can someone help me resolve this problem? |
@jkugsiya As Fetch Upstream is something GitHub has released on the repository directly, but this will not push to your PR. This is only updating your branch on the fork, not pushed into the PR. |
How can I push to the PR then? As all the changes are done and there's nothing to commit more. |
You need to check if your local PR branch is synced with the repository or not. If not then git pull and update the PR, and then you need to push to this PR. |
It automatically appeared now, without doing anything. It could have been an issue with GitHub. Thanks @anandwana001 !! |
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 @jkugsiya. This LGTM.
@anandwana001 I think you're requesting changes which is blocking this PR--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.
LGTM
Explanation
Fixes #3062 : Added label to AdministratorControlsActivity.
Checklist
Screenshot of successful test: