Fix part of #3258 : Replacing CircularImageView with ShapeableImageView #4139

bhaktideshmukh · 2022-01-28T12:00:24Z

Explanation

Fix part of #3258 : Replacing CircularImageView with ShapeableImageView.

Screenshots

Phone portrait mode

Before	After

Phone landscape mode

Before	After

Tablet portrait mode

Before	After

Tablet landscape mode

Before	After

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

For UI-specific PRs only

If your PR includes UI-related changes, then:

Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

bhaktideshmukh · 2022-01-28T13:06:12Z

PTAL @BenHenning
Thanks.

BenHenning

Thanks @bhaktideshmukh! Apologies for the delayed review.

I just took a first pass--PTAL. I also think that you have failing tests. Please make sure that affected tests are passing (I think one of my comments will help with this).

BenHenning · 2022-02-01T02:56:07Z

app/src/main/res/layout-land/profile_edit_fragment.xml

@@ -33,7 +33,7 @@
        app:layout_constraintStart_toStartOf="parent"
        app:layout_constraintTop_toTopOf="parent" />

-      <de.hdodenhof.circleimageview.CircleImageView
+      <com.google.android.material.imageview.ShapeableImageView


This & later changes don't require the appearance overlay property to be set?

I added the appearance overlay property here.

app/src/main/res/layout-sw600dp-land/profile_chooser_add_view.xml

BenHenning · 2022-02-01T02:58:50Z

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

@@ -459,4 +459,9 @@
    <item name="android:gravity">center_horizontal</item>
  </style>

+  <!-- ShapeableImageView -->
+  <style name="ShapeAppearanceOverlay.App.Circular" parent="">


I believe you can omit parent if it isn't defined.

BenHenning · 2022-02-01T02:59:41Z

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

@@ -459,4 +459,9 @@
    <item name="android:gravity">center_horizontal</item>
  </style>

+  <!-- ShapeableImageView -->
+  <style name="ShapeAppearanceOverlay.App.Circular" parent="">
+    <item name="cornerSize">50%</item>


Is there documentation that you can point to explaining this property?

Also, I'm not sure about the style & item names (both in terms of formatting consistency and the way the style name is broken up). @rt4914 what are your thoughts?

https://howtodoandroid.com/shapeableimageview-material-components-android/

Given other styles names that we have like
TextInputLayout,
TextInputEditText,
AppTabTextAppearance

I think ShapeableImageView.Circular can be a good name because it clearly mentions where to use this theme (ShapeableImageView) and also what will be the output of the theme (Circular).

app/build.gradle

BenHenning · 2022-02-01T03:02:00Z

Also @rt4914 I noticed in the screenshots that the 'add profile' button isn't aligned with the others--is this a recent regression?

bhaktideshmukh · 2022-02-01T17:03:17Z

PTAL @BenHenning
Thanks

oppiabot · 2022-02-01T17:06:20Z

Unassigning @bhaktideshmukh since a re-review was requested. @bhaktideshmukh, please make sure you have addressed all review comments. Thanks!

rt4914 · 2022-02-02T10:59:28Z

Also @rt4914 I noticed in the screenshots that the 'add profile' button isn't aligned with the others--is this a recent regression?

@BenHenning Its a known issue, mentioned in #2116

rt4914

@bhaktideshmukh Please rename the PR title and description to mention ``Fix part of #instead ofFix #...` because once this PR is approved you will need to do similar changes in other places too like `ProfileProgress`, `ProfileList`, `ProfileEdit` and in last you will need to remove the existing `CircularImageView` library mentioned in the issue.

rt4914 · 2022-02-02T11:02:55Z

@bhaktideshmukh Please add screenshot of MDPIP device and API 21 too.

…ageView

bhaktideshmukh · 2022-02-02T14:43:22Z

@bhaktideshmukh Please rename the PR title and description to mention ``Fix part of #instead ofFix #...` because once this PR is approved you will need to do similar changes in other places too like `ProfileProgress`, `ProfileList`, `ProfileEdit` and in last you will need to remove the existing `CircularImageView` library mentioned in the issue.

Done!

bhaktideshmukh · 2022-02-02T14:46:53Z

@bhaktideshmukh Please add screenshot of MDPIP device and API 21 too.

Here for MDPI API 21 is not available.
The least level here starts with 22.
So what should I do?
Should I add the screenshot for API 22?

bhaktideshmukh · 2022-02-02T14:49:16Z

@rt4914 PTAL
Thanks

oppiabot · 2022-02-02T14:52:20Z

Unassigning @bhaktideshmukh since a re-review was requested. @bhaktideshmukh, please make sure you have addressed all review comments. Thanks!

rt4914 · 2022-02-03T08:59:54Z

@bhaktideshmukh Please add screenshot of MDPIP device and API 21 too.

Here for MDPI API 21 is not available. The least level here starts with 22. So what should I do? Should I add the screenshot for API 22?

@bhaktideshmukh Select the second tab which is x86 images in that you will find the 21 version.

bhaktideshmukh · 2022-02-03T10:51:26Z

MDPI device and API 21

Phone portrait mode

Before	After

Phone landscape mode

Before	After

bhaktideshmukh · 2022-02-03T10:51:52Z

@bhaktideshmukh Please add screenshot of MDPIP device and API 21 too.

Done!!

bhaktideshmukh · 2022-02-03T10:52:23Z

PTAL @rt4914
Thanks.

BenHenning · 2022-02-04T05:05:13Z

Apologies @bhaktideshmukh but I'll need to close this since you force pushed the branch. Please open a new PR.

In the future, you should never force push as it can break comment history. See step 3 under https://github.com/oppia/oppia-android/wiki/Making-a-code-change#making-a-code-change-using-the-terminal (though I suggest reviewing all those instructions to make sure you know our process).

Once the PR is recreated, we can take another review pass on it.

bhaktideshmukh · 2022-02-04T06:14:02Z

Apologies @bhaktideshmukh but I'll need to close this since you force pushed the branch. Please open a new PR.

In the future, you should never force push as it can break comment history. See step 3 under https://github.com/oppia/oppia-android/wiki/Making-a-code-change#making-a-code-change-using-the-terminal (though I suggest reviewing all those instructions to make sure you know our process).

Once the PR is recreated, we can take another review pass on it.

I apologize for my mistake. I read the instructions.
I have also recreated the PR for this issue.

bhaktideshmukh requested review from BenHenning and rt4914 as code owners January 28, 2022 12:00

oppiabot bot assigned BenHenning Jan 28, 2022

BenHenning requested changes Feb 1, 2022

View reviewed changes

BenHenning assigned bhaktideshmukh and unassigned BenHenning Feb 1, 2022

bhaktideshmukh force-pushed the shapable_image_view branch from 5fa7648 to e07e458 Compare February 1, 2022 09:51

rt4914 self-assigned this Feb 1, 2022

oppiabot bot assigned BenHenning and unassigned bhaktideshmukh Feb 1, 2022

bhaktideshmukh force-pushed the shapable_image_view branch from b76ed47 to 80dc56e Compare February 2, 2022 05:06

rt4914 suggested changes Feb 2, 2022

View reviewed changes

rt4914 assigned bhaktideshmukh and unassigned rt4914 Feb 2, 2022

bhaktideshmukh added 3 commits February 2, 2022 20:05

Fix part of oppia#3258 : Replacing CircularImageView with ShapeableIm…

549b067

…ageView

Updated bazel version

1b5429f

Updated maven_install.json

8cda1f2

bhaktideshmukh force-pushed the shapable_image_view branch from 80dc56e to 8cda1f2 Compare February 2, 2022 14:37

bhaktideshmukh changed the title ~~Fix #3258 : Replacing CircularImageView with ShapeableImageView~~ Fix part of #3258 : Replacing CircularImageView with ShapeableImageView Feb 2, 2022

oppiabot bot assigned rt4914 and unassigned bhaktideshmukh Feb 2, 2022

rt4914 assigned bhaktideshmukh Feb 3, 2022

BenHenning closed this Feb 4, 2022

BenHenning mentioned this pull request Feb 5, 2022

Fix part of #3258 : Replacing CircularImageView with ShapeableImageView #4155

Merged

6 tasks

rt4914 mentioned this pull request Feb 25, 2022

Fix #4104: Fixed profile edit image view #4215

Closed

6 tasks

Rd4dev mentioned this pull request Feb 24, 2024

Find a maintained replacement for CircularImageView #3258

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #3258 : Replacing CircularImageView with ShapeableImageView #4139

Fix part of #3258 : Replacing CircularImageView with ShapeableImageView #4139

bhaktideshmukh commented Jan 28, 2022 •

edited

Loading

bhaktideshmukh commented Jan 28, 2022

BenHenning left a comment

BenHenning Feb 1, 2022

bhaktideshmukh Feb 1, 2022

BenHenning Feb 1, 2022

BenHenning Feb 1, 2022

bhaktideshmukh Feb 1, 2022

rt4914 Feb 2, 2022

BenHenning commented Feb 1, 2022

bhaktideshmukh commented Feb 1, 2022

oppiabot bot commented Feb 1, 2022

rt4914 commented Feb 2, 2022

rt4914 left a comment

rt4914 commented Feb 2, 2022

bhaktideshmukh commented Feb 2, 2022

bhaktideshmukh commented Feb 2, 2022

bhaktideshmukh commented Feb 2, 2022

oppiabot bot commented Feb 2, 2022

rt4914 commented Feb 3, 2022

bhaktideshmukh commented Feb 3, 2022

bhaktideshmukh commented Feb 3, 2022

bhaktideshmukh commented Feb 3, 2022 •

edited

Loading

BenHenning commented Feb 4, 2022

bhaktideshmukh commented Feb 4, 2022

Fix part of #3258 : Replacing CircularImageView with ShapeableImageView #4139

Fix part of #3258 : Replacing CircularImageView with ShapeableImageView #4139

Conversation

bhaktideshmukh commented Jan 28, 2022 • edited Loading

Explanation

Screenshots

Essential Checklist

For UI-specific PRs only

bhaktideshmukh commented Jan 28, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Feb 1, 2022

Choose a reason for hiding this comment

bhaktideshmukh Feb 1, 2022

Choose a reason for hiding this comment

BenHenning Feb 1, 2022

Choose a reason for hiding this comment

BenHenning Feb 1, 2022

Choose a reason for hiding this comment

bhaktideshmukh Feb 1, 2022

Choose a reason for hiding this comment

rt4914 Feb 2, 2022

Choose a reason for hiding this comment

BenHenning commented Feb 1, 2022

bhaktideshmukh commented Feb 1, 2022

oppiabot bot commented Feb 1, 2022

rt4914 commented Feb 2, 2022

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 commented Feb 2, 2022

bhaktideshmukh commented Feb 2, 2022

bhaktideshmukh commented Feb 2, 2022

bhaktideshmukh commented Feb 2, 2022

oppiabot bot commented Feb 2, 2022

rt4914 commented Feb 3, 2022

bhaktideshmukh commented Feb 3, 2022

bhaktideshmukh commented Feb 3, 2022

bhaktideshmukh commented Feb 3, 2022 • edited Loading

BenHenning commented Feb 4, 2022

bhaktideshmukh commented Feb 4, 2022

bhaktideshmukh commented Jan 28, 2022 •

edited

Loading

bhaktideshmukh commented Feb 3, 2022 •

edited

Loading