ANDROID-13941 Implement Callout Image/Avatar #317

fjquileze · 2023-11-10T17:45:03Z

🥅 What's the goal?

We need to add the image/avatar to callout element as it's specified here: Callout Component Specs

Currently we only have icons

🚧 How do we do it?

Add an image and avatar (circular image) to CalloutView
Add the imageRes and imageConfig params to the Callout composable maintaining the old iconRes for retrocompatibility purposes

☑️ Checks

I updated the documentation, including readmes and wikis. If this is a breaking change, update UPGRADING.md to inform users how to proceed. If no updates are necessary, indicate so.
Tested with dark mode.
Tested with API 23.

🧪 How can I test this?

🖼️ Screenshots/Video

Screen_recording_20231110_184246.mp4

Mistica App QR or download link
Reviewed by Mistica design team

github-actions · 2023-11-10T17:49:52Z

📱 New catalog for testing generated: Download

DevPabloGarcia

Thanks! check comments please

DevPabloGarcia · 2023-11-15T07:52:01Z

library/src/main/res/layout/callout_view.xml

+				/>
+
+		<ImageView
+				android:id="@+id/callout_image"


Suggestion: Set visibility default as: GONE

DevPabloGarcia · 2023-11-15T07:52:36Z

library/src/main/res/layout/callout_view.xml

+				android:visibility="gone"
+				app:shapeAppearance="@style/CircleImageView"
+				tools:src="@tools:sample/avatars"
+				tools:visibility="gone"


as we have android:visibility="gone", tools:visibility="gone" is not needed

nice catch! I'm going to remove it

DevPabloGarcia · 2023-11-15T07:57:42Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

@@ -322,6 +389,11 @@ class CalloutView @JvmOverloads constructor(
        const val BUTTONS_CONFIG_SECONDARY_LINK = 4
        const val BUTTONS_CONFIG_LINK = 5

+        const val IMAGE_CONFIG_NONE = -1


Question, why not use a enum class?

I was just following the code that was here before but I can make an enum

DevPabloGarcia · 2023-11-15T08:01:41Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

@@ -31,6 +38,8 @@ fun Callout(
    description: String?,
    buttonConfig: CalloutButtonConfig,
    @DrawableRes iconRes: Int? = null,


IMHO instead of having one attribute to icon an other to imageRes we can simplify it, and just have iconRes

you're right, I'm gonna change it!

DevPabloGarcia · 2023-11-15T08:04:23Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

@@ -56,13 +65,43 @@ fun Callout(
            modifier = Modifier.padding(16.dp),
            verticalAlignment = Alignment.Top,
        ) {
-            iconRes?.let {
+
+            if (iconRes != null) {


I don't understand this logic, first we check if iconRes is not null to create an Image, but bellow if imageConfig is IMAGE_CONFIG_ICON we create the same object?

I think we can check first the imageConfig and then create the image:

`when (imageConfig) {
CalloutView.IMAGE_CONFIG_ICON -> // create icon
CalloutView.IMAGE_CONFIG_SQUARE -> // create square image
CalloutView.IMAGE_CONFIG_CIRCULAR -> //create circular image

}
`

github-actions · 2023-11-15T10:45:51Z

📱 New catalog for testing generated: Download

jeslat · 2023-11-16T08:29:26Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

+    BindingMethod(
+        type = CalloutView::class,
+        attribute = "calloutImage",
+        method = "setImage",
+    ),
+    BindingMethod(
+        type = CalloutView::class,
+        attribute = "calloutCircularImage",
+        method = "setCircularImage",
+    ),


I think the API would be easier to use if you follow the same approach that we use in ListRowView or DataCardView. Instead of having one method per image type, which can be confusing for the user because he has to know which types of images support this component, have two methods:

setAsset

setAssetType

If we have more asset types in the future, will be easier to add them.

I'm not sure if maintaining the retro compatibility in this case is worth it. We could remove the setIcon method and migrate its uses (it should be easy).

It would be the same that you have done in the Compose version

Good point, I'm going to change it

jeslat · 2023-11-16T08:33:42Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

+                    }
+                    CalloutViewImageConfig.IMAGE_CONFIG_SQUARE -> {
+                        resourceIconPainter(
+                            iconRes = iconRes, iconType = IconType.SQUARE_IMAGE,


Move the iconType to the next line

jeslat · 2023-11-16T08:34:26Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

+                            modifier = Modifier.padding(end = 16.dp)
+                        )
+                    }
+                    else -> noIcon()


I would not use the else, if in the future you add a new type, the compiler won't tell you anything.

good point! changed

jeslat · 2023-11-16T08:36:28Z

library/src/main/java/com/telefonica/mistica/compose/callout/Callout.kt

-                    colorFilter = ColorFilter.tint(MisticaTheme.colors.neutralHigh)
-                )
+
+            if (iconRes != null) {


I would extract the icon logic to a new private composable:

if (iconRes != null) { Icon(imageConfig, iconRes) }

github-actions · 2023-11-20T18:43:57Z

📱 New catalog for testing generated: Download

jeslat · 2023-11-21T09:05:27Z

library/src/main/res/values/attrs_components.xml

@@ -272,7 +272,6 @@
    <declare-styleable name="CalloutView">
        <attr name="calloutTitle" format="string"/>
        <attr name="calloutDescription" format="string"/>
-        <attr name="calloutIcon" format="reference"/>


As this is a breaking change, remember to deploy a major release and add a new section in the migrations document https://github.com/Telefonica/mistica-android/blob/main/UPGRADING.md

Change also the callout readme to add these attributes https://github.com/Telefonica/mistica-android/tree/main/library/src/main/java/com/telefonica/mistica/callout

jeslat · 2023-11-21T09:08:35Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

@@ -37,7 +38,12 @@ import com.telefonica.mistica.util.getThemeColor
    BindingMethod(
        type = CalloutView::class,
        attribute = "calloutIcon",


These attributes have to have the same name as defined in attrs_components

jeslat · 2023-11-21T09:12:39Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

+                    image.visibility = GONE
+                    circularImage.visibility = GONE


I would move these visibility changes inside setIcon method. The same with setImage and setCircularImage

jeslat · 2023-11-21T09:13:41Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

+            CalloutViewImageConfig.IMAGE_CONFIG_NONE -> {
+                icon.visibility = GONE
+                image.visibility = GONE
+                circularImage.visibility = GONE
+            }
+
+            CalloutViewImageConfig.IMAGE_CONFIG_ICON -> {
+                icon.visibility = VISIBLE
+                image.visibility = GONE
+                circularImage.visibility = GONE
+            }
+
+            CalloutViewImageConfig.IMAGE_CONFIG_SQUARE -> {
+                icon.visibility = GONE
+                image.visibility = VISIBLE
+                circularImage.visibility = GONE
+            }
+
+            CalloutViewImageConfig.IMAGE_CONFIG_CIRCULAR -> {
+                icon.visibility = GONE
+                image.visibility = GONE
+                circularImage.visibility = VISIBLE


I think you are doing these visibility changes two times, I would remove this method

DevPabloGarcia · 2023-11-21T10:00:52Z

catalog/src/main/java/com/telefonica/mistica/catalog/ui/compose/components/Callouts.kt

@@ -153,3 +184,9 @@ fun Callouts() {
    }
 }

+private enum class ImageTypes(val iconType: CalloutViewImageConfig) {


I found we have 3 enums for the same reason: Image Type, I think we can simplify it only using CalloutViewImageConfig.

To show the name in callout catalog we can make a function or extension function to obtain the name

I agree, I'm gonna refactor it

DevPabloGarcia · 2023-11-21T10:02:05Z

...rc/main/java/com/telefonica/mistica/catalog/ui/classic/components/CalloutsCatalogFragment.kt

            }
+            setAssetType(imageConfig)


If the imageConfig is NONE we call this function twice, we can avoid this moving this line to each condition

Suggested change

setAssetType(imageConfig)

when (imageConfig) {

CalloutViewImageConfig.IMAGE_CONFIG_NONE -> {

setAssetType(imageConfig)

}

CalloutViewImageConfig.IMAGE_CONFIG_ICON -> {

setAsset(R.drawable.ic_callout)

setAssetType(imageConfig)

}

CalloutViewImageConfig.IMAGE_CONFIG_SQUARE,

CalloutViewImageConfig.IMAGE_CONFIG_CIRCULAR -> {

setAsset(R.drawable.media_card_sample_image)

setAssetType(imageConfig)

}

}

DevPabloGarcia · 2023-11-21T10:02:33Z

...rc/main/java/com/telefonica/mistica/catalog/ui/classic/components/CalloutsCatalogFragment.kt

@@ -95,4 +119,11 @@ class CalloutsCatalogFragment : Fragment() {
        SECONDARY_AND_LINK(CalloutView.BUTTONS_CONFIG_SECONDARY_LINK),
        LINK(CalloutView.BUTTONS_CONFIG_LINK),
    }
+
+    private enum class CalloutImageConfig(val imageConfig: CalloutViewImageConfig) {


Se next comment, we are declaring 3 enums to store the same value

DevPabloGarcia · 2023-11-21T10:03:21Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

-            icon.setImageResource(iconRes)
-            icon.visibility = VISIBLE
+    fun setAsset(@DrawableRes assetRes: Int?) {
+        asset = assetRes


Suggested change

asset = assetRes

when (assetType) {

null,

CalloutViewImageConfig.IMAGE_CONFIG_NONE -> noAsset()

CalloutViewImageConfig.IMAGE_CONFIG_ICON -> setIcon(assetRes)

CalloutViewImageConfig.IMAGE_CONFIG_SQUARE -> setImage(assetRes)

CalloutViewImageConfig.IMAGE_CONFIG_CIRCULAR -> setCircularImage(assetRes)

}

I don't get this change, can you explain it a little more?

yceballost · 2023-11-21T11:17:20Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

this left padding is not correct when the image is "none" (in classic)

An example in web
https://tinyurl.com/ykhg3dk8

yceballost · 2023-11-21T11:50:14Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

Image is not using border radius token

This screenshot is in Vivo new for example:

Image component has to use mediaSmall
https://github.com/Telefonica/mistica-design/blob/5542488dfa4d6e6555b63129cc33e2ea502e9c39/tokens/movistar.json#L1118

fixed! 👍

github-actions · 2023-11-24T15:05:45Z

📱 New catalog for testing generated: Download

…vatar

github-actions · 2023-11-27T11:59:52Z

📱 New catalog for testing generated: Download

Co-authored-by: fjquileze <[email protected]>

github-actions · 2023-11-28T09:44:42Z

📱 New catalog for testing generated: Download

github-actions · 2023-11-28T11:12:49Z

📱 New catalog for testing generated: Download

jeslat · 2023-11-28T14:08:09Z

library/src/main/java/com/telefonica/mistica/callout/CalloutView.kt

            BUTTONS_CONFIG_NONE -> {}
        }
    }

+    private fun setImageConfig(imageConfig: CalloutViewImageConfig) {


You are not using this method

I'm using it at the CalloutCatalogFragment to set the type

No, you are not, you are using setButtonsConfig. This method is private, it's impossible that you are using it out of this class

oh! you're right, I was thinking in the setAssetType method. I'm gonna delete this one

jeslat · 2023-11-28T16:04:03Z

catalog/src/main/java/com/telefonica/mistica/catalog/ui/compose/components/Callouts.kt

+                    imageConfig = when (iconType) {
+                        CalloutViewImageConfig.NONE -> CalloutViewImageConfig.NONE
+                        CalloutViewImageConfig.ICON -> CalloutViewImageConfig.ICON
+                        CalloutViewImageConfig.SQUARE_IMAGE -> CalloutViewImageConfig.SQUARE_IMAGE
+                        CalloutViewImageConfig.CIRCULAR_IMAGE -> CalloutViewImageConfig.CIRCULAR_IMAGE
+                    },


You don't need the when, both types are the same

🤦‍♂️ fixed!

github-actions · 2023-11-28T16:13:51Z

📱 New catalog for testing generated: Download

jeslat

👍

DevPabloGarcia

Nice !

…vatar

nimeacuerdo · 2023-12-01T08:58:08Z

Hi @yceballost! anything left on your side to approve this?

github-actions · 2023-12-01T09:02:50Z

📱 New catalog for testing generated: Download

yceballost

LGTM! Thanks!

fjquileze added 5 commits November 8, 2023 18:07

implement callout image xml version

9fbac0f

implement callout image compose version

7c800a5

implement callout circular image xml version

5cf99ed

refactor

7c75982

refactor callouts image implementation and make it retrocompatible

389e625

fjquileze requested review from DevPabloGarcia, a team and jeslat and removed request for a team November 10, 2023 17:45

DevPabloGarcia reviewed Nov 15, 2023

View reviewed changes

PR changes

32d6a21

fjquileze requested review from DevPabloGarcia, a team and yceballost and removed request for a team November 15, 2023 11:11

jeslat reviewed Nov 16, 2023

View reviewed changes

fjquileze added 2 commits November 20, 2023 19:33

PR changes - refactor to only 2 methods

ecb7422

PR changes

9dc79ea

fjquileze requested a review from jeslat November 21, 2023 08:26

jeslat reviewed Nov 21, 2023

View reviewed changes

DevPabloGarcia reviewed Nov 21, 2023

View reviewed changes

yceballost requested changes Nov 21, 2023

View reviewed changes

PR changes

ad03053

fjquileze added 2 commits November 27, 2023 11:00

Merge branch 'main' into task/android-13941-implement-callout-image-a…

3a6bdb9

…vatar

update documentation

094efd7

Update design tokens (#319)

5de22c9

Co-authored-by: fjquileze <[email protected]>

use mediaSmallBorderRadius token for callout square images

feb14ab

fjquileze requested review from jeslat, DevPabloGarcia and yceballost November 28, 2023 12:45

jeslat reviewed Nov 28, 2023

View reviewed changes

PR changes

9cf2cf3

jeslat approved these changes Nov 29, 2023

View reviewed changes

DevPabloGarcia approved these changes Nov 29, 2023

View reviewed changes

Merge branch 'main' into task/android-13941-implement-callout-image-a…

d31bdba

…vatar

yceballost approved these changes Dec 1, 2023

View reviewed changes

fjquileze merged commit 05f5567 into main Dec 1, 2023
5 checks passed

fjquileze deleted the task/android-13941-implement-callout-image-avatar branch December 1, 2023 10:26

-        asset = assetRes
+        when (assetType) {
+                null,
+                CalloutViewImageConfig.IMAGE_CONFIG_NONE -> noAsset()
+                CalloutViewImageConfig.IMAGE_CONFIG_ICON -> setIcon(assetRes)
+                CalloutViewImageConfig.IMAGE_CONFIG_SQUARE -> setImage(assetRes)
+                CalloutViewImageConfig.IMAGE_CONFIG_CIRCULAR -> setCircularImage(assetRes)
+            }

ANDROID-13941 Implement Callout Image/Avatar #317

ANDROID-13941 Implement Callout Image/Avatar #317

Conversation

fjquileze commented Nov 10, 2023

🥅 What's the goal?

🚧 How do we do it?

☑️ Checks

🧪 How can I test this?

github-actions bot commented Nov 10, 2023

DevPabloGarcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeslat Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceballost Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 27, 2023

github-actions bot commented Nov 28, 2023

github-actions bot commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 28, 2023

jeslat left a comment

Choose a reason for hiding this comment

DevPabloGarcia left a comment

Choose a reason for hiding this comment

nimeacuerdo commented Dec 1, 2023

github-actions bot commented Dec 1, 2023

yceballost left a comment

Choose a reason for hiding this comment

jeslat Nov 16, 2023 •

edited

Loading

yceballost Nov 21, 2023 •

edited

Loading