-
Notifications
You must be signed in to change notification settings - Fork 5
ANDROID-17239 Compose MediaCard imagePosition left/right #481
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
Conversation
| private fun applyLinkTextFix(text: String): String { | ||
| return text.padEnd(18, ' ') | ||
| } |
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.
Moved to an extension and reused in the MediaCard
| CardActions(primaryButton, linkButton) | ||
| } | ||
| } | ||
| MediaCardImagePosition.Left -> { |
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.
New Left image position
| } | ||
| } | ||
| } | ||
| MediaCardImagePosition.Right -> { |
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.
Right image position
| sealed class MediaCardImage(val contentDescription: String?) { | ||
| class MediaCardImageResource(@DrawableRes val imageRes: Int, contentDescription: String? = null) : MediaCardImage(contentDescription) | ||
| class MediaCardImageBitmap(val imageBitmap: ImageBitmap, contentDescription: String? = null) : MediaCardImage(contentDescription) | ||
| public sealed class MediaCardImage(public val contentDescription: String?) { |
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 public visibility explicitely
| public class MediaCardImageBitmap(public val imageBitmap: ImageBitmap, contentDescription: String? = null) : MediaCardImage(contentDescription) | ||
| } | ||
|
|
||
| public enum class MediaCardImagePosition { |
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.
New enum for the image position
| imagePosition: MediaCardImagePosition = MediaCardImagePosition.Top, | ||
| imageContentScale: ContentScale = ContentScale.Crop, |
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.
New parameters to MediaCard
| } | ||
| linkButton?.let { | ||
| Button( | ||
| modifier = if (primaryButton == null) Modifier.offset(x = (-8).dp) else Modifier, |
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.
Now we can use invalidatePaddings and invalidateMinWidth so the offset(x = (-8).dp) hack is no longer needed.
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.
Pull request overview
This PR extends the MediaCard component to support left and right image positioning in addition to the existing top position, aligning with updated design specifications.
Key Changes
- Added
imagePositionparameter toMediaCardto control image placement (Top, Left, Right) - Added
imageContentScaleparameter to allow customization of image scaling behavior - Extracted link text padding utility function to shared location for reuse across card components
Reviewed changes
Copilot reviewed 6 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
MediaCard.kt |
Added image position variants with different layouts and made API classes public |
Card.kt |
Enhanced to support vertical button orientation and configurable padding for flexible layouts |
HighLightedCard.kt |
Refactored to use shared link text padding utility |
TextUtils.kt |
Added shared utility function for link text padding |
MediaCardTest.kt |
Added comprehensive parameterized tests for all image positions and button combinations |
MediaCards.kt |
Updated catalog demo to include image position selector |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
library/src/main/java/com/telefonica/mistica/compose/card/mediacard/MediaCard.kt
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
…magePosition' into feature/ANDROID-17239-MediaCardImagePosition
|
[Firebase] 📱 New catalog for testing generated: |
| CardImage( | ||
| mediaCardImage = image, | ||
| modifier = Modifier | ||
| .width(150.dp) |
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.
could be this mediaCardImage width extracted to a const val?
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 687c1f5
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.
Should it be implemented in xml too? Also media card readme should be updated
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.
I think xml can be done in a different task if needed as we did lately in other tasks like when applying accessibility in views and in compose separately.
Also, doing it in xml when we know for sure we won't need this as we do everything new in compose I'm not sure it makes sense. It would probably never be needed/used by anybody.
I've added a couple of images to the readme and improved that README.md
pmartinbTEF
left a comment
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.
Good job! just a couple of questions
|
[Firebase] 📱 New catalog for testing generated: |
juangardi21
left a comment
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.
LGTM, nice job!
pmartinbTEF
left a comment
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.
Thanks
|
[Firebase] 📱 New catalog for testing generated: |
AnaMontes11
left a comment
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.
lgtm
🥅 What's the goal?
Update MediaCard so that it also handles imagePosition to left and right.
Specs:
https://www.figma.com/design/tKdPOfcUALzVIh5oizFbm7/%F0%9F%94%B8-Cards-Specs?node-id=11252-17819&t=07VsJqXhuGmrwcIq-4
Examples:
https://www.figma.com/design/WCkDDzlXE16R6yXaljxddj/M%C3%ADstica-Mobile?node-id=54711-14370&t=v3KwV70ZvdIgGul4-4
🚧 How do we do it?
☑️ Checks
🧪 How can I test this?
Before.mov
After.mov
[Firebase] 📱 New catalog for testing generatedcomment)