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

ANDROID-14134 Set assets clickable #324

Merged
merged 6 commits into from
Dec 26, 2023
Merged

ANDROID-14134 Set assets clickable #324

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f765cc6
set assets clickable
jeprubio Dec 22, 2023
435b934
ANDROID-14134 Update icon size
jeprubio Dec 22, 2023
da4e9d6
ANDROID-14134 Revert update icon size
jeprubio Dec 26, 2023
ba3ca7f
ANDROID-14134 remove space
jeprubio Dec 26, 2023
c599cb4
ANDROID-14134 remove unused parameter
jeprubio Dec 26, 2023
b56750c
ANDROID-14134 add clickable row examples
jeprubio Dec 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions ...rc/main/java/com/telefonica/mistica/catalog/ui/classic/components/ListsCatalogFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ class ListsCatalogFragment : Fragment() {

val boxedInverseList: MisticaRecyclerView = view.findViewById(R.id.boxed_inverse_list)
boxedInverseList.adapter = ListAdapter(backgroundType = ListRowView.BackgroundType.TYPE_BOXED_INVERSE)

val clickableRow: MisticaRecyclerView = view.findViewById(R.id.clickable_list)
clickableRow.adapter = ClickableListAdapter(backgroundType = ListRowView.BackgroundType.TYPE_NORMAL)
}

class ListAdapter(
Expand Down Expand Up @@ -553,6 +556,31 @@ class ListsCatalogFragment : Fragment() {

class ListViewHolder(val rowView: ListRowView) : RecyclerView.ViewHolder(rowView)

class ClickableListAdapter(
@ListRowView.BackgroundType private val backgroundType: Int,
Copy link
Member

Choose a reason for hiding this comment

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

This parameter are not being used!

Copy link
Contributor Author

@jeprubio jeprubio Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

True, it's not needed at least for now, removed from here the catalog. Thanks.

) : RecyclerView.Adapter<ListViewHolder>() {
override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ListViewHolder {
return ListViewHolder(
LayoutInflater.from(parent.context).inflate(
R.layout.screen_fragment_lists_catalog_item,
parent,
false
) as ListRowView
)
}

override fun getItemCount(): Int = 1

override fun onBindViewHolder(holder: ListViewHolder, position: Int) {
with(holder.rowView) {
setTitle("Clickable Asset")
setAssetOnClickListener {
Copy link
Member

@dagonco dagonco Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From the way I have understood Figma, the list row should be clickable. cc: @AnaMontes11 is that true?

Otherwise, probably is a good idea to have all or atleast some of the list row variants with this new modification.
This is the intention of the catalog. A sample of the capabilities that each Mística's component has.

Copy link
Contributor Author

@jeprubio jeprubio Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The row can be clickable, that's for sure, and that remains the same as it was.

Added an extra row which is clickable the row and the asset to each of the catalogs if it's that what you mean.
Screenshot 2023-12-26 at 12 35 32

When clicking on the row:
Screenshot 2023-12-26 at 12 43 04

And on the asset:
Screenshot 2023-12-26 at 12 43 13

Toast.makeText(context, "Asset clicked!", Toast.LENGTH_SHORT).show()
}
}
}
}

private companion object {
const val IMAGE_URL = "https://www.fotoaparat.cz/imgs/a/26/2639/0n1wjdf0-cr-em13-09-1200x627x9.jpg"
}
Expand Down
40 changes: 40 additions & 0 deletions catalog/src/main/java/com/telefonica/mistica/catalog/ui/compose/components/Lists.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package com.telefonica.mistica.catalog.ui.compose.components

import android.widget.Toast
import androidx.compose.foundation.Image
import androidx.compose.foundation.background
import androidx.compose.foundation.border
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.fillMaxSize
import androidx.compose.foundation.layout.fillMaxWidth
Expand All @@ -13,6 +15,7 @@ import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material.Divider
import androidx.compose.material.ExperimentalMaterialApi
import androidx.compose.material.MaterialTheme
import androidx.compose.material.Switch
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
Expand All @@ -23,6 +26,7 @@ import androidx.compose.ui.layout.ContentScale
import androidx.compose.ui.platform.LocalContext
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import coil.compose.rememberAsyncImagePainter
import coil.request.ImageRequest
import coil.transform.CircleCropTransformation
Expand Down Expand Up @@ -298,10 +302,14 @@ const val IMAGE_URL = "https://www.fotoaparat.cz/imgs/a/26/2639/0n1wjdf0-cr-em13
@Composable
fun Lists() {
val samples = samples()
val context = LocalContext.current
Copy link
Member

Choose a reason for hiding this comment

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

This is used only once, there is no need to declare it as a property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but can't ve moved to the same line it's used as that would not compile:
Screenshot 2023-12-26 at 12 07 00

LazyColumn(
modifier = Modifier
.fillMaxSize(),
) {
item {
SectionTitle("Full Width List")
}
items(samples) { item ->
ListRowItem(
backgroundType = item.backgroundType,
Expand All @@ -321,6 +329,9 @@ fun Lists() {
color = MisticaTheme.colors.divider
)
}
item {
SectionTitle("Boxed List")
}
items(samples.map {
it.copy(backgroundType = BackgroundType.TYPE_BOXED)
}) { item ->
Expand All @@ -338,6 +349,9 @@ fun Lists() {
bottom = item.bottom,
)
}
item {
SectionTitle("Boxed Inverse List")
}
items(samples.map {
it.copy(
backgroundType = BackgroundType.TYPE_BOXED_INVERSE,
Expand All @@ -358,6 +372,23 @@ fun Lists() {
bottom = item.bottom,
)
}
item {
SectionTitle("Clickable Asset")
ListRowItem(
title = "Clickable Asset",
subtitle = "Subtitle",
description = "Description",
isBadgeVisible = true,
badge = "1",
listRowIcon = ListRowIcon.CircleIcon(
painterResource(id = R.drawable.ic_lists),
backgroundColor = MisticaTheme.colors.backgroundAlternative,
modifier = Modifier.clickable {
Toast.makeText(context, "Asset Clicked", Toast.LENGTH_SHORT).show()
}
),
)
}
}
}

Expand Down Expand Up @@ -389,6 +420,15 @@ fun Avatar(url: String) {
)
}

@Composable
private fun SectionTitle(title: String) {
Text(
text = title.uppercase(),
style = MaterialTheme.typography.h6.copy(fontSize = 14.sp),
modifier = Modifier.padding(16.dp)
)
}

@Composable
private fun CustomSlot() {
Box(
Expand Down
19 changes: 19 additions & 0 deletions catalog/src/main/res/layout/screen_fragment_lists_catalog.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,24 @@
app:listLayoutType="boxed"
/>

<com.telefonica.mistica.title.TitleView
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_gravity="start"
android:layout_marginStart="16dp"
android:layout_marginTop="8dp"
app:title="Clickable Asset"
/>

<com.telefonica.mistica.list.MisticaRecyclerView
android:nestedScrollingEnabled="false"
android:id="@+id/clickable_list"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_marginTop="8dp"
android:layout_weight="0.5"
app:listLayoutType="full_width"
/>

</LinearLayout>
</androidx.core.widget.NestedScrollView>
16 changes: 11 additions & 5 deletions library/src/main/java/com/telefonica/mistica/compose/list/ListRowIcon.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,31 +28,36 @@ sealed class ListRowIcon(val contentDescription: String?) {
data class NormalIcon(
val painter: Painter? = null,
private val description: String? = null,
val modifier: Modifier = Modifier,
Copy link
Contributor Author

@jeprubio jeprubio Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In compose we expose the modifier which is a common used practice which we were not following until now: https://chrisbanes.me/posts/always-provide-a-modifier/

) : ListRowIcon(description)

data class CircleIcon(
val painter: Painter? = null,
val backgroundColor: Color = Color.Transparent,
private val description: String? = null,
val modifier: Modifier = Modifier,
) : ListRowIcon(description)

data class SmallAsset(
val painter: Painter? = null,
private val description: String? = null,
val modifier: Modifier = Modifier,
) : ListRowIcon(description)

data class LargeAsset(
val painter: Painter? = null,
val aspectRatio: AspectRatio = AspectRatio.RATIO_1_1,
val contentScale: ContentScale = ContentScale.Crop,
private val description: String? = null,
val modifier: Modifier = Modifier,
) : ListRowIcon(description)

data class ImageAsset(
val painter: Painter? = null,
val dimensions: ImageDimensions? = null,
val contentScale: ContentScale = ContentScale.Crop,
private val description: String? = null,
val modifier: Modifier = Modifier,
) : ListRowIcon(description)

enum class AspectRatio(val width: Dp, val height: Dp) {
Expand All @@ -75,9 +80,9 @@ sealed class ListRowIcon(val contentDescription: String?) {
@Composable
private fun NormalIcon.DrawNormalIcon() {
Box(
modifier = Modifier
modifier = modifier
.size(40.dp)
.wrapContentSize(align = Alignment.Center)
.wrapContentSize(align = Alignment.Center),
) {
painter?.let {
Icon(
Expand All @@ -94,6 +99,7 @@ sealed class ListRowIcon(val contentDescription: String?) {
private fun CircleIcon.DrawCircleIcon() {
Circle(
color = backgroundColor,
modifier = modifier,
) {
painter?.let {
Icon(
Expand All @@ -110,7 +116,7 @@ sealed class ListRowIcon(val contentDescription: String?) {
Image(
painter = painter,
contentDescription = contentDescription,
modifier = Modifier
modifier = modifier
.size(40.dp)
Copy link

Choose a reason for hiding this comment

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

For accessibility reasons, we might consider expanding the clickable area of the asset to 48px. We have updated our specs.

Copy link
Contributor Author

@jeprubio jeprubio Dec 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I'm fine with that if the specs have also been updated. Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just exposing the modifier and not making it clickable with just that. Also doing that includes breaking changes in the lib. In my opinion we should ensure what we want to make clickable or not on the designs and then use this modifier to make it clickable or not depending on those designs. I'm going to revert this change.

.clip(CircleShape),
contentScale = ContentScale.Crop,
Expand All @@ -124,7 +130,7 @@ sealed class ListRowIcon(val contentDescription: String?) {
Image(
painter = painter,
contentDescription = contentDescription,
modifier = Modifier
modifier = modifier
.height(aspectRatio.height)
.width(aspectRatio.width)
.clip(RoundedCornerShape(4.dp)),
Expand All @@ -139,7 +145,7 @@ sealed class ListRowIcon(val contentDescription: String?) {
Image(
painter = painter,
contentDescription = contentDescription,
modifier = Modifier
modifier = modifier
.width(dimensions?.width?.dp ?: dimensionResource(id = R.dimen.asset_default_size))
.height(dimensions?.height?.dp ?: dimensionResource(id = R.dimen.asset_default_size))
.clip(RoundedCornerShape(4.dp)),
Expand Down
3 changes: 2 additions & 1 deletion library/src/main/java/com/telefonica/mistica/compose/shape/Circle.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ import com.telefonica.mistica.compose.theme.MisticaTheme

@Composable
fun Circle(
modifier: Modifier = Modifier,
color: Color = MisticaTheme.colors.neutralLow,
size: Dp = 40.dp,
content: @Composable (() -> Unit),
) {
Box(
modifier = Modifier
modifier = modifier
.size(size)
.clip(CircleShape)
.background(color)
Expand Down
4 changes: 4 additions & 0 deletions library/src/main/java/com/telefonica/mistica/list/ListRowView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,10 @@ class ListRowView @JvmOverloads constructor(
}
}

fun setAssetOnClickListener(clickListener: OnClickListener) {
assetImageLayout.setOnClickListener(clickListener)
}
Comment on lines +312 to +314
Copy link
Contributor Author

@jeprubio jeprubio Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is the code added for classic views in order to be able to set the click listener.


private fun updateIconVisibility() {
assetCircularImageView.isVisible = assetType == TYPE_IMAGE
assetRoundedImageView.isVisible = assetType == TYPE_IMAGE_1_1 || assetType == TYPE_IMAGE_7_10
Expand Down
26 changes: 25 additions & 1 deletion library/src/test/java/com/telefonica/mistica/compose/list/ListRowItemKtTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package com.telefonica.mistica.compose.list

import androidx.compose.foundation.clickable
import androidx.compose.material.ExperimentalMaterialApi
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.res.painterResource
import androidx.compose.ui.test.hasTestTag
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.compose.ui.test.onRoot
import androidx.compose.ui.test.performClick
import com.telefonica.mistica.compose.shape.Chevron
import com.telefonica.mistica.compose.tag.Tag
import com.telefonica.mistica.compose.theme.MisticaTheme
Expand All @@ -15,6 +20,9 @@ import org.junit.Test
import org.junit.runner.RunWith
import org.robolectric.RobolectricTestRunner
import com.telefonica.mistica.R
import org.junit.Assert.assertEquals

private const val LIST_ROW_ITEM_ASSET_TAG = "listRowItemAssetTag"

@RunWith(RobolectricTestRunner::class)
internal class ListRowItemKtTest: ScreenshotsTest() {
Expand All @@ -35,14 +43,30 @@ internal class ListRowItemKtTest: ScreenshotsTest() {
`then screenshot is OK`()
}

@Test
fun `check ListRowItem with clickable asset`() {
var clicked = 0
Copy link
Member

Choose a reason for hiding this comment

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

This is acting as a boolean, not as an integer

Copy link
Contributor Author

@jeprubio jeprubio Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not exactly as I want to know that it has been invoked only once. A boolean would only ensure it has been invoked but it could be multiple times. By doing it this way the condition is stronger.

Copy link
Member

Choose a reason for hiding this comment

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

The condition is exactly the same, you are checking if it has been clicked 1 time instead of 0.

val onAssetClick: () -> Unit = {
clicked++
}
`when ListRowItem with asset`(
dimensions = ImageDimensions(width = 44, height = 44),
onAssetClick = onAssetClick,
)
composeTestRule.onNode(hasTestTag(LIST_ROW_ITEM_ASSET_TAG)).performClick()

assertEquals(1, clicked)
}
Comment on lines +46 to +59
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests that the asset is clickable on compose


@OptIn(ExperimentalMaterialApi::class)
private fun `when ListRowItem with asset`(dimensions: ImageDimensions) {
private fun `when ListRowItem with asset`(dimensions: ImageDimensions, onAssetClick: () -> Unit = {}) {
composeTestRule.setContent {
MisticaTheme(brand = MovistarBrand) {
ListRowItem(
listRowIcon = ListRowIcon.ImageAsset(
painter = painterResource(id = R.drawable.placeholder),
dimensions = ImageDimensions(width = dimensions.width, height = dimensions.height),
modifier = Modifier.testTag(LIST_ROW_ITEM_ASSET_TAG).clickable { onAssetClick() },
),
headline = Tag("Promo"),
isBadgeVisible = true,
Expand Down
17 changes: 17 additions & 0 deletions library/src/test/java/com/telefonica/mistica/list/ListRowViewTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import androidx.test.ext.junit.rules.activityScenarioRule
import com.telefonica.mistica.DummyActivity
import com.telefonica.mistica.R
import com.telefonica.mistica.testutils.ScreenshotsTest
import org.junit.Assert.assertEquals
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
Expand All @@ -26,4 +27,20 @@ internal class ListRowViewTest: ScreenshotsTest() {
compareScreenshot(Espresso.onView(ViewMatchers.withId(R.id.dummy_activity_wrapper)))
}
}

@Test
fun `check ListRowView xml onClick`() {
rule.scenario.onActivity { activity ->
var clicks = 0
Copy link
Member

Choose a reason for hiding this comment

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

Same here, it is used as a boolean

Copy link
Contributor Author

@jeprubio jeprubio Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Same answer here 😄

val wrapper: FrameLayout = activity.findViewById(R.id.dummy_activity_wrapper)
activity.layoutInflater.inflate(R.layout.test_list_row_view, wrapper, true)

with (activity.findViewById<ListRowView>(R.id.list_row_view_32)) {
setOnClickListener { clicks++ }
performClick()
}

assertEquals(1, clicks)
}
}
Comment on lines +31 to +45
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests that the asset is clickable on xml

}
4 changes: 4 additions & 0 deletions library/src/test/res/layout/test_list_row_view.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
android:orientation="vertical">

<com.telefonica.mistica.list.ListRowView
android:id="@+id/list_row_view_32"
android:layout_width="match_parent"
android:layout_height="wrap_content"
app:listRowActionLayout="@layout/list_row_chevron_action"
Expand All @@ -20,6 +21,7 @@
app:listRowAssetHeight="32dp"
app:listRowAssetWidth="32dp" />
<com.telefonica.mistica.list.ListRowView
android:id="@+id/list_row_view_64"
android:layout_width="match_parent"
android:layout_height="wrap_content"
app:listRowActionLayout="@layout/list_row_chevron_action"
Expand All @@ -33,6 +35,7 @@
app:listRowAssetHeight="64dp"
app:listRowAssetWidth="64dp" />
<com.telefonica.mistica.list.ListRowView
android:id="@+id/list_row_view_default_sizes"
android:layout_width="match_parent"
android:layout_height="wrap_content"
app:listRowActionLayout="@layout/list_row_chevron_action"
Expand All @@ -44,6 +47,7 @@
app:listRowIsBoxed="true"
app:listRowTitle="Image 64 x 64 (default sizes)" />
<com.telefonica.mistica.list.ListRowView
android:id="@+id/list_row_view_image_1_1"
android:layout_width="match_parent"
android:layout_height="wrap_content"
app:listRowActionLayout="@layout/list_row_chevron_action"
Expand Down
Loading