-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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-14098 set asset dimensions #322
Changes from 6 commits
8ce1102
68ee6ec
396f739
2aa3ecc
f56282c
537782a
1c1ee2b
cb172eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package com.telefonica.mistica.list.model | ||
|
||
data class ImageDimensions( | ||
val width: Int, | ||
val height: Int, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,13 @@ | |
<enum name="image_1_1" value="3" /> | ||
<enum name="image_7_10" value="4" /> | ||
<enum name="image_16_9" value="5" /> | ||
<enum name="image_rounded" value="6" /> | ||
</attr> | ||
<attr name="listRowAssetHeight" format="integer"> | ||
<enum name="undefined" value="64" /> | ||
</attr> | ||
<attr name="listRowAssetWidth" format="integer"> | ||
<enum name="undefined" value="64" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should these attrs be of type "dimension" in case someone uses them as follows? listRowAssetWidth="@dimen/some_width" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1, I think it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, I can do this now. Good shout. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, both in 1c1ee2b |
||
</attr> | ||
<!-- {@deprecated This attribute is deprecated. Use <code>listRowBackgroundType</code> instead } --> | ||
<attr name="listRowIsBoxed" format="boolean" /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
package com.telefonica.mistica.compose.list | ||
|
||
import androidx.compose.material.ExperimentalMaterialApi | ||
import androidx.compose.ui.res.painterResource | ||
import androidx.compose.ui.test.junit4.createComposeRule | ||
import androidx.compose.ui.test.onRoot | ||
import com.telefonica.mistica.compose.shape.Chevron | ||
import com.telefonica.mistica.compose.tag.Tag | ||
import com.telefonica.mistica.compose.theme.MisticaTheme | ||
import com.telefonica.mistica.compose.theme.brand.MovistarBrand | ||
import com.telefonica.mistica.list.model.ImageDimensions | ||
import com.telefonica.mistica.testutils.ScreenshotsTest | ||
import org.junit.Rule | ||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.robolectric.RobolectricTestRunner | ||
import com.telefonica.mistica.R | ||
|
||
@RunWith(RobolectricTestRunner::class) | ||
internal class ListRowItemKtTest: ScreenshotsTest() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious, why that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you create a test with Android Studio it creates them this way sometimes, I'm not sure which rules does it follow. I can remove that if you want. |
||
@get:Rule | ||
val composeTestRule = createComposeRule() | ||
|
||
@Test | ||
fun `check ListRowItem with 64x64 asset`() { | ||
`when ListRowItem with asset`(ImageDimensions(width = 64, height = 64)) | ||
|
||
`then screenshot is OK`() | ||
} | ||
|
||
@Test | ||
fun `check ListRowItem with 32x32 asset`() { | ||
`when ListRowItem with asset`(ImageDimensions(width = 32, height = 32)) | ||
|
||
`then screenshot is OK`() | ||
} | ||
|
||
@OptIn(ExperimentalMaterialApi::class) | ||
private fun `when ListRowItem with asset`(dimensions: ImageDimensions) { | ||
composeTestRule.setContent { | ||
MisticaTheme(brand = MovistarBrand) { | ||
ListRowItem( | ||
listRowIcon = ListRowIcon.ImageAsset( | ||
painter = painterResource(id = R.drawable.placeholder), | ||
dimensions = ImageDimensions(width = dimensions.width, height = dimensions.height), | ||
), | ||
headline = Tag("Promo"), | ||
isBadgeVisible = true, | ||
title = "Title", | ||
subtitle = "Subtitle", | ||
description = "Description", | ||
trailing = { Chevron() }, | ||
) | ||
} | ||
} | ||
} | ||
|
||
private fun `then screenshot is OK`() { | ||
compareScreenshot(composeTestRule.onRoot()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appart from programatic configuration we expose also configurable elements with xml attributes. Check ListRowView styleable in
attrs_components.xml
to add new asset type enum and height/width attributes. Ideally if databinding method matches attribute definition behaviour, its better, so there's no difference in the usage when using databinding and when not.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I forgot doing that, I'll do that now. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 396f739
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it working the binding method with this new parameter? Or is it just ignored because is nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one of the fun setAssetType will be null with binding and there are the other two of setAssetHeight and setAssetWidth to set the dimensions. That param is in order to use it when creating the element programmatically, but can be changed, it was thought as a shortcut.