Fixes #3900 : Added toolbar to exit ProfilePictureActivity #3918

bkaur-bkj · 2021-10-09T17:14:51Z

Explanation

Fixes #3900: Added toolbar to exit the ProfilePictureActivity

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).

After adding the toolbar and shadow

BenHenning

Thanks @bkaur-bkj. Left some initial thoughts--PTAL.

app/src/main/java/org/oppia/android/app/profileprogress/ProfilePictureActivityPresenter.kt

app/src/main/res/values/strings.xml

app/src/main/java/org/oppia/android/app/profileprogress/ProfilePictureActivityPresenter.kt

BenHenning

Thanks @bkaur-bkj. I think the approach overall LGTM. The main gap yet is ensuring that we have tests to ensure that this stays working long-term.

app/src/main/java/org/oppia/android/app/profileprogress/ProfilePictureActivityPresenter.kt

app/src/main/res/values/strings.xml

app/src/main/java/org/oppia/android/app/profileprogress/ProfilePictureActivityPresenter.kt

bkaur-bkj · 2021-10-13T17:34:27Z

@rt4914 @BenHenning Sir as said I have tried adding the test case for the above cases of checking title and back button working but as I am not aware much of writing test cases my case for action button check is getting failed, can you pls help what should I change here.

BenHenning

Thanks @bkaur-bkj. Just had a few other comments. Beyond that:

Fix part of #3602: Added label for ProfilePictureActivity #3881 was just merged; could you sync this PR with the latest develop changes, address the conflict, and update the PR to use the activity title string introduced in that PR, instead (so that we don't need to duplicate it)?
I see there was a lint error. Do you have your local Git repository set up with the pre-push hook to catch lint errors? If not, you should run setup.sh locally as explained here. That will ensure lint failures are caught before pushing which will help you discover & fix them more quickly (i.e. without needing to wait for CI or running them manually).

app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfilePictureActivityTest.kt

BenHenning · 2021-10-14T05:31:32Z

Also, @bkaur-bkj please make sure that you respond to all comments before sending a PR back for review. It's important to make it clear to reviewers that you've explicitly addressed a comment so that we know to check for it being resolved in our next review round.

bkaur-bkj · 2021-10-14T06:22:13Z

Also, @bkaur-bkj please make sure that you respond to all comments before sending a PR back for review. It's important to make it clear to reviewers that you've explicitly addressed a comment so that we know to check for it being resolved in our next review round.

okay sorry for not replying last time

rt4914

@bkaur-bkj PTAL

rt4914 · 2021-10-14T06:50:31Z

app/src/main/res/layout/profile_picture_activity.xml

+      layout="@layout/toolbar"
+      app:layout_constraintEnd_toEndOf="parent"
+      app:layout_constraintStart_toStartOf="parent"
+      app:layout_constraintTop_toTopOf="parent" />


Lets make it a bit simple.
Instead of using include, write code similar to this

oppia-android/app/src/main/res/layout/app_version_activity.xml

Line 9 in 15669dc

<com.google.android.material.appbar.AppBarLayout

This way you can actually remove a lot of kotlin code.

Also from the same link above you will need to introduce [View](

oppia-android/app/src/main/res/layout/app_version_activity.xml

Line 32 in 15669dc

<View

) in your xml to show the shadow below the toolbar.

FYI I have suggested to remove include because we won't be using them in future in our code as per #3746

Okay sir done

@rt4914 I have made these changes and also the one asked for adding tests, but still the test fails and I have no clue how to modify them.

bkaur-bkj · 2021-10-15T17:56:24Z

@BenHenning @rt4914 PTAL

BenHenning · 2021-10-16T09:20:17Z

Sorry, I'll need to review this on Monday.

rt4914

@bkaur-bkj PTAL thanks.

app/src/main/res/layout/profile_picture_activity.xml

app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfilePictureActivityTest.kt

bkaur-bkj · 2021-10-16T14:02:32Z

@rt4914 PTAL I made the changes requested and have also added the updated screenshot , but with just that naming changes one check is failing and I unable to resolve it

rt4914

@bkaur-bkj PTAL at one comment.

app/src/main/java/org/oppia/android/app/profileprogress/ProfilePictureActivityPresenter.kt

app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfilePictureActivityTest.kt

BenHenning

Thanks @bkaur-bkj. LGTM!

BenHenning · 2021-10-19T03:09:12Z

Restarting Gradle run since it was a Java 8 flake.

bkaur-bkj · 2021-10-20T14:04:04Z

@rt4914 PTAL I have removed the unnecessary part of toolbar

rt4914

LGTM, thanks.

added toolbar to exit ProfilePictureActivity

f58f510

bkaur-bkj requested review from BenHenning and rt4914 as code owners October 9, 2021 17:14

bkaur-bkj assigned rt4914 and BenHenning Oct 9, 2021

BenHenning requested changes Oct 12, 2021

View reviewed changes

BenHenning assigned bkaur-bkj and unassigned BenHenning Oct 12, 2021

changed string name and function name

4b1dc89

bkaur-bkj assigned BenHenning and unassigned bkaur-bkj Oct 12, 2021

bkaur-bkj added 2 commits October 12, 2021 20:59

changed the string name

06de2fa

m

1115592

BenHenning requested changes Oct 13, 2021

View reviewed changes

BenHenning assigned bkaur-bkj and unassigned BenHenning Oct 13, 2021

added test cases

1a9c829

bkaur-bkj assigned BenHenning and unassigned bkaur-bkj Oct 13, 2021

BenHenning requested changes Oct 14, 2021

View reviewed changes

BenHenning assigned bkaur-bkj and unassigned BenHenning Oct 14, 2021

rt4914 suggested changes Oct 14, 2021

View reviewed changes

rt4914 removed their assignment Oct 14, 2021

bkaur-bkj added 3 commits October 14, 2021 12:24

Merge branch 'develop' into add_toolbar_to_exit_ProfilePictureActivity

1ab4c47

modified toolbar code

3cadf5e

modified tests

9cf8567

bkaur-bkj assigned BenHenning and rt4914 and unassigned bkaur-bkj Oct 15, 2021

Lint checks correction

a553b06

rt4914 suggested changes Oct 16, 2021

View reviewed changes

rt4914 assigned bkaur-bkj and unassigned rt4914 and BenHenning Oct 16, 2021

toolbar shadow editted

d31b211

bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Oct 16, 2021

rt4914 suggested changes Oct 18, 2021

View reviewed changes

app/src/main/java/org/oppia/android/app/profileprogress/ProfilePictureActivityPresenter.kt Outdated Show resolved Hide resolved

app/src/sharedTest/java/org/oppia/android/app/profileprogress/ProfilePictureActivityTest.kt Show resolved Hide resolved

rt4914 assigned BenHenning and bkaur-bkj and unassigned rt4914 Oct 18, 2021

rt4914 mentioned this pull request Oct 18, 2021

Fix part of #3602 Added label for HomeActivity #3850

Merged

6 tasks

BenHenning approved these changes Oct 19, 2021

View reviewed changes

BenHenning removed their assignment Oct 19, 2021

Removing uneccessary code changes

e289843

bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Oct 20, 2021

rt4914 approved these changes Oct 21, 2021

View reviewed changes

rt4914 merged commit 43aee2e into oppia:develop Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #3900 : Added toolbar to exit ProfilePictureActivity #3918

Fixes #3900 : Added toolbar to exit ProfilePictureActivity #3918

bkaur-bkj commented Oct 9, 2021 •

edited

BenHenning left a comment

BenHenning left a comment

bkaur-bkj commented Oct 13, 2021

BenHenning left a comment •

edited

BenHenning commented Oct 14, 2021

bkaur-bkj commented Oct 14, 2021

rt4914 left a comment

rt4914 Oct 14, 2021

rt4914 Oct 14, 2021

bkaur-bkj Oct 14, 2021

bkaur-bkj Oct 14, 2021

bkaur-bkj commented Oct 15, 2021

BenHenning commented Oct 16, 2021

rt4914 left a comment

bkaur-bkj commented Oct 16, 2021 •

edited

rt4914 left a comment

BenHenning left a comment

BenHenning commented Oct 19, 2021

bkaur-bkj commented Oct 20, 2021

rt4914 left a comment

Fixes #3900 : Added toolbar to exit ProfilePictureActivity #3918

Fixes #3900 : Added toolbar to exit ProfilePictureActivity #3918

Conversation

bkaur-bkj commented Oct 9, 2021 • edited

Explanation

Essential Checklist

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

bkaur-bkj commented Oct 13, 2021

BenHenning left a comment • edited

Choose a reason for hiding this comment

BenHenning commented Oct 14, 2021

bkaur-bkj commented Oct 14, 2021

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 Oct 14, 2021

Choose a reason for hiding this comment

rt4914 Oct 14, 2021

Choose a reason for hiding this comment

bkaur-bkj Oct 14, 2021

Choose a reason for hiding this comment

bkaur-bkj Oct 14, 2021

Choose a reason for hiding this comment

bkaur-bkj commented Oct 15, 2021

BenHenning commented Oct 16, 2021

rt4914 left a comment

Choose a reason for hiding this comment

bkaur-bkj commented Oct 16, 2021 • edited

rt4914 left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Oct 19, 2021

bkaur-bkj commented Oct 20, 2021

rt4914 left a comment

Choose a reason for hiding this comment

bkaur-bkj commented Oct 9, 2021 •

edited

BenHenning left a comment •

edited

bkaur-bkj commented Oct 16, 2021 •

edited