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.

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

APPS-7260 Add option to support the three types of assets defined by design #32

Merged
merged 13 commits into from
Dec 1, 2020

Conversation

jeslat
Copy link
Contributor

@jeslat jeslat commented Nov 27, 2020

Here you can see the definition https://app.zeplin.io/project/5d653c69f828bf7299c551c1/screen/5f7eed32afc7924fc7b12432/

I've removed the listRowHasSmallAsset attribute and added a new listRowAssetType which supports three types:

  • image (circle image 40x40)
  • smallIcon (icon 20x20)
  • largeIcon (icon 20x20 with a circle background of 40x40)

You can see here the result:

imagen

Comment on lines -57 to +65
attribute = "listRowIsBoxed",
method = "setBoxed"
attribute = "listRowAssetType",
method = "setAssetType"
),
BindingMethod(
type = ListRowView::class,
attribute = "listRowHasSmallAsset",
method = "setSmallAsset"
attribute = "listRowIsBoxed",
method = "setBoxed"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The diff is a bit weird because I've changed the order but the result is that I've removed listRowHasSmallAsset and added listRowAssetType

private fun getAssetResource(withAsset: Boolean, withAssetType: Int): Int? =
if (withAsset) {
if (withAssetType == TYPE_IMAGE) {
R.drawable.netflix_logo
Copy link
Contributor

@dpastor dpastor Nov 30, 2020

Choose a reason for hiding this comment

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

Minor detail. But maybe we can use a simple vector asset here? Just to avoid the 13kb of it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can use highlighted_card_custom_background.png which is already in the catalog.

android:layout_gravity="center"
android:scaleType="centerCrop"
android:visibility="gone"
app:shapeAppearance="@style/CircleImageView"
Copy link
Contributor

@dpastor dpastor Nov 30, 2020

Choose a reason for hiding this comment

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

Awesome!! 😄 Really nice job!!

1.0.8
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to commit this (but there is no problem if you do it), the action will commit the version you set automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, perfect

Copy link
Contributor

@gmerinojimenez gmerinojimenez left a comment

Choose a reason for hiding this comment

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

Nice job!!

@jeslat jeslat merged commit 96d2f2b into master Dec 1, 2020
@jeslat jeslat deleted the apps/APPS-7260-asset-background branch December 1, 2020 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants