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 3868 : Add label for splash activity #3868

Merged
merged 3 commits into from
Oct 7, 2021
Merged

#Fixes 3868 : Add label for splash activity #3868

merged 3 commits into from
Oct 7, 2021

Conversation

bkaur-bkj
Copy link
Contributor

Explanation

Fix part of #3602 Added Label for Splash Activity
Added label and Testcase and removed the file name from exempted list.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

@bkaur-bkj bkaur-bkj requested a review from BenHenning as a code owner October 1, 2021 10:24
rt4914
rt4914 suggested changes Oct 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.

@BenHenning PTAL

app/src/main/res/values/strings.xml Outdated
@@ -461,6 +461,7 @@
<string name="correct_submitted_answer_with_append">Correct submitted answer: %s</string>
<string name="incorrect_submitted_answer">Incorrect submitted answer</string>
<string name="incorrect_submitted_answer_with_append">Incorrect submitted answer: %s</string>
<string name="splash_activity_title">Splash Activity</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of Splash Activity this should be Oppia. @BenHenning Any thoughts on this one?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, 'Oppia' sounds correct. We should use the existing untranslated string for that purpose (https://github.com/oppia/oppia-android/blob/develop/app/src/main/res/values/untranslated_strings.xml#L3).

@oppiabot
Copy link

oppiabot bot commented Oct 3, 2021

Hi @bkaur-bkj, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

@bkaur-bkj bkaur-bkj changed the title added label for splash activity #FIxes 3868 : Add label for splash activity Oct 4, 2021
@bkaur-bkj bkaur-bkj changed the title #FIxes 3868 : Add label for splash activity #Fixes 3868 : Add label for splash activity Oct 4, 2021
BenHenning
BenHenning reviewed Oct 5, 2021
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Leaving one follow-up comment.

app/src/main/res/values/strings.xml Outdated
@@ -461,6 +461,7 @@
<string name="correct_submitted_answer_with_append">Correct submitted answer: %s</string>
<string name="incorrect_submitted_answer">Incorrect submitted answer</string>
<string name="incorrect_submitted_answer_with_append">Incorrect submitted answer: %s</string>
<string name="splash_activity_title">Splash Activity</string>
Copy link
Member

Choose a reason for hiding this comment

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

Yes, 'Oppia' sounds correct. We should use the existing untranslated string for that purpose (https://github.com/oppia/oppia-android/blob/develop/app/src/main/res/values/untranslated_strings.xml#L3).

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Oct 5, 2021
@bkaur-bkj
Copy link
Contributor Author

@rt4914 sir, PTAL I not understood why these checks are failing

rt4914
rt4914 suggested changes Oct 5, 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.

@bkaur-bkj PTAL.
Make sure you always run the test cases if you have added/updated tests.

app/src/sharedTest/java/org/oppia/android/app/splash/SplashActivityTest.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Oct 5, 2021
@rt4914
Copy link
Contributor

rt4914 commented Oct 5, 2021

@bkaur-bkj Can you run the test case on Espresso and Robolectirc and share the screenshots showing they are passing. https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing

@bkaur-bkj
Copy link
Contributor Author

bkaur-bkj commented Oct 6, 2021 •
edited
Loading

@bkaur-bkj Can you run the test case on Espresso and Robolectirc and share the screenshots showing they are passing. https://github.com/oppia/oppia-android/wiki/Oppia-Android-Testing

eXPRESSO CASES

Test cases result on Expresso, Splash activity has Correct Label test has passed

Robolectric

Test cases result on Robolectric, 3 ignored but Splash activity has Correct Label test has passed

@rt4914 rt4914 assigned rt4914 and unassigned bkaur-bkj Oct 6, 2021
rt4914
rt4914 approved these changes Oct 6, 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 assigned BenHenning and unassigned rt4914 Oct 6, 2021
BenHenning
BenHenning approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @bkaur-bkj. LGTM!

@oppiabot
Copy link

oppiabot bot commented Oct 7, 2021

Unassigning @BenHenning since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Oct 7, 2021
@BenHenning
Copy link
Member

Merging since CI is now passing (I needed to restart a few stalled workflows).

@BenHenning BenHenning merged commit daf4716 into oppia:develop Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees
No one assigned
Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bkaur-bkj @rt4914 @BenHenning