Fix #2825: Added label for profile rename activity #2928

justdvnsh · 2021-03-16T10:31:43Z

Explanation

Fix #2825 Added label for profile rename activity to read the title "Profile Rename Activity" when talkback is on

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

justdvnsh · 2021-03-16T10:33:04Z

@prayutsu @rt4914 PTAL

prayutsu

@justdvnsh Generally LGTM, just suggested a few nit changes and fix the newly added test case.

app/src/main/AndroidManifest.xml

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt

prayutsu · 2021-03-17T05:26:01Z

@justdvnsh Also please go through our Guidance On Submitting A PR once.
And please address the conversation comments by replying "Done" if you have made the change. Also, go through our Instructions for making a Code changeand please choose a good name for future PRs as per the instructions.

Thanks!

justdvnsh · 2021-03-17T05:31:02Z

@prayutsu Sure I'll keep that in mind. For the PR name , I was following the reference PR which was mentioned in the issue.

prayutsu · 2021-03-17T05:41:43Z

@justdvnsh Thanks for addressing all the comments, you can now resolve all conversations initiated by me.
And your PR name is perfect, I am just emphasizing the branch name e.g. the branch name in the reference PR is - a11y-faq-list and we use the acronym a11y for accessibility so it is a good example.

prayutsu

LGTM, Thanks @justdvnsh
Just had one doubt for @rt4914 , is it okay to use the existing string @profile_rename_title or a new one for the label should be created?

rt4914

@justdvnsh Suggested changes. PTAL

app/src/main/AndroidManifest.xml

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt

…the string

rt4914

Suggested nit change.

app/src/main/java/org/oppia/android/app/settings/profile/ProfileRenameActivityPresenter.kt

…eded

rt4914

@justdvnsh Suggested nit change.

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt

rt4914

LGTM, thanks.

justdvnsh · 2021-03-19T15:10:18Z

@rt4914 the tests are failing ? Just by adding a single space ? What is that about ??

rt4914 · 2021-03-19T16:36:59Z

@rt4914 the tests are failing ? Just by adding a single space ? What is that about ??

@justdvnsh Its not because of space. I am inspecting the code to know the reason of failure.

Also in future PRs please use small characters and - only for branch names.

justdvnsh · 2021-03-20T01:54:24Z

@rt4914 Yep, it passed now.

Sure. Will keep in mind from future PR's

BenHenning

Thanks @justdvnsh! Just had one nit. PTAL.

BenHenning · 2021-03-20T03:20:28Z

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt

+      )
+    )
+
+    val title = activityTestRule.activity.title


Please add a comment above this like the one listed here to provide some context about what this check is attempting to do.

@justdvnsh The comment that you added does not match with the one mentioned in the reference. Please use same comment for better clarity and consistency.

BenHenning

Thanks @justdvnsh. I think something might have gone wrong with the commit history in the latest commit--PTAL at my comment.

BenHenning · 2021-03-25T03:15:00Z

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt

+  application = ProfileRenameActivityTest.TestApplication::class,
+  qualifiers = "port-xxhdpi"
+)
+class ProfileRenameActivityTest {


@justdvnsh it appears the entire file has been changed. Did you by any chance upload the file using Windows line endings? We generally use Unix line endings everywhere (e.g. rather than ). I suggest looking at https://stackoverflow.com/questions/2517190/how-do-i-force-git-to-use-lf-instead-of-crlf-under-windows if you are using Windows.

We don't want to mark the entire file as changed in this way.

@BenHenning Sure. I'll make sure to fix this issue by today itself.

BenHenning · 2021-03-30T04:34:39Z

@justdvnsh are you still working on this issue?

justdvnsh · 2021-03-30T09:04:12Z

@BenHenning Yeah. Sorry, I was not as regular to this as usual. Actually, was trying out few things for the feature I want to work on in my proposal. But I'll make sure to fix this by today

justdvnsh · 2021-03-30T16:27:17Z

@BenHenning @rt4914 Created a new PR here.

#3029

added label for profile rename activity

2b40d74

justdvnsh requested review from BenHenning and rt4914 as code owners March 16, 2021 10:31

prayutsu suggested changes Mar 16, 2021

View reviewed changes

app/src/main/AndroidManifest.xml Show resolved Hide resolved

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt Outdated Show resolved Hide resolved

prayutsu reviewed Mar 16, 2021

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt Outdated Show resolved Hide resolved

prayutsu assigned justdvnsh Mar 16, 2021

Improved Code Formatting and Fixed the failing CI test

14ae620

prayutsu approved these changes Mar 17, 2021

View reviewed changes

rt4914 assigned rt4914 and unassigned justdvnsh Mar 17, 2021

rt4914 suggested changes Mar 17, 2021

View reviewed changes

rt4914 assigned justdvnsh and unassigned rt4914 Mar 17, 2021

justdvnsh added 3 commits March 17, 2021 22:49

changed the activity title string label and changed all instances of …

e1b4fff

…the string

changed the context to application context

287c32f

fixed the failing CI test

69da742

rt4914 assigned rt4914 and unassigned justdvnsh Mar 18, 2021

rt4914 suggested changes Mar 18, 2021

View reviewed changes

app/src/main/java/org/oppia/android/app/settings/profile/ProfileRenameActivityPresenter.kt Outdated Show resolved Hide resolved

rt4914 assigned justdvnsh and unassigned rt4914 Mar 18, 2021

removed title from ProfileRenameActivityPresenter since it was not ne…

a325d23

…eded

rt4914 suggested changes Mar 19, 2021

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileRenameActivityTest.kt Outdated Show resolved Hide resolved

rt4914 assigned BenHenning and unassigned justdvnsh Mar 19, 2021

modified code formatting

613d489

rt4914 approved these changes Mar 19, 2021

View reviewed changes

BenHenning reviewed Mar 20, 2021

View reviewed changes

BenHenning assigned justdvnsh Mar 20, 2021

added comment for readbility

0c9f893

rt4914 unassigned BenHenning Mar 22, 2021

added comment for better redability of code

30dd002

rt4914 assigned BenHenning and unassigned justdvnsh Mar 24, 2021

BenHenning requested changes Mar 25, 2021

View reviewed changes

BenHenning assigned justdvnsh Mar 25, 2021

rt4914 unassigned BenHenning Mar 30, 2021

prayutsu closed this Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #2825: Added label for profile rename activity #2928

Fix #2825: Added label for profile rename activity #2928

justdvnsh commented Mar 16, 2021

justdvnsh commented Mar 16, 2021

prayutsu left a comment •

edited

Loading

prayutsu commented Mar 17, 2021

justdvnsh commented Mar 17, 2021

prayutsu commented Mar 17, 2021

prayutsu left a comment

rt4914 left a comment

rt4914 left a comment

rt4914 left a comment

rt4914 left a comment

justdvnsh commented Mar 19, 2021

rt4914 commented Mar 19, 2021

justdvnsh commented Mar 20, 2021

BenHenning left a comment

BenHenning Mar 20, 2021

justdvnsh Mar 21, 2021

rt4914 Mar 22, 2021

BenHenning left a comment

BenHenning Mar 25, 2021

justdvnsh Mar 30, 2021

BenHenning commented Mar 30, 2021

justdvnsh commented Mar 30, 2021

justdvnsh commented Mar 30, 2021

Fix #2825: Added label for profile rename activity #2928

Fix #2825: Added label for profile rename activity #2928

Conversation

justdvnsh commented Mar 16, 2021

Explanation

Checklist

justdvnsh commented Mar 16, 2021

prayutsu left a comment • edited Loading

Choose a reason for hiding this comment

prayutsu commented Mar 17, 2021

justdvnsh commented Mar 17, 2021

prayutsu commented Mar 17, 2021

prayutsu left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 left a comment

Choose a reason for hiding this comment

justdvnsh commented Mar 19, 2021

rt4914 commented Mar 19, 2021

justdvnsh commented Mar 20, 2021

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Mar 20, 2021

Choose a reason for hiding this comment

justdvnsh Mar 21, 2021

Choose a reason for hiding this comment

rt4914 Mar 22, 2021

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Mar 25, 2021

Choose a reason for hiding this comment

justdvnsh Mar 30, 2021

Choose a reason for hiding this comment

BenHenning commented Mar 30, 2021

justdvnsh commented Mar 30, 2021

justdvnsh commented Mar 30, 2021

prayutsu left a comment •

edited

Loading