-
Notifications
You must be signed in to change notification settings - Fork 991
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
MBL-2069 Add images on rewards UI #2224
Conversation
} | ||
Column( | ||
modifier = Modifier.padding(dimensions.paddingMediumLarge) | ||
) { |
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.
Please ignore everything after this line. Only relevant changes in this file are up to here. The diff just did a terrible job of detecting actual differences.
@@ -44,6 +50,7 @@ fun KSRewardCardPreview() { | |||
conversion = "about $400", | |||
title = "Deck of cards", | |||
backerCountBadgeText = "23 backers", | |||
image = Photo.builder().altText("").full("https://i.kickstarter.com/Superbacker_Lock-up-0e9d240.png").build(), |
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.
Was trying to get an image to show in the compose preview but it didn't work. If anyone has ideas lmk
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.
For preview to render needs to be a local resource I'm afraid, you can:
- try to add the file path to a local resource (local to the internal builds if wanna to a real image).
- or whichever library we are using for the image add a placeholder for the empty use case, chances are it will render on the preview
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, empty placeholder renders if I pass in an empty string url
shape = RoundedCornerShape(dimensions.radiusMediumSmall), | ||
) { | ||
Column( | ||
modifier = Modifier.background(colors.kds_white) | ||
) { | ||
if (yourSelectionIsVisible) { |
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 the Your Selection tag to its own composable.
@@ -60,6 +60,9 @@ data class KSDimensions( | |||
val storedCardImageWidth: Dp = Dp.Unspecified, | |||
val alertIconSize: Dp = Dp.Unspecified, | |||
val plotChargeItemWidth: Dp = Dp.Unspecified, | |||
val cardWidth: Dp = Dp.Unspecified, | |||
val cardImageHeight: Dp = Dp.Unspecified, | |||
val cardImageAspectRatio: Float = Float.NaN, |
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.
For the rewards carousel where we use a fixed card width, use the cardWidth
and cardImageHeight
dimens. For the add ons screen and manage pledge screen, the card width is match_parent, so use cardImageAspectRatio
dimen instead.
@@ -545,6 +545,7 @@ interface BackingFragmentViewModel { | |||
|
|||
private fun joinProjectDataAndReward(projectData: ProjectData): Pair<ProjectData, Reward> { | |||
val reward = projectData.backing()?.reward() | |||
?: projectData.project().backing()?.reward() |
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.
Please sanity check me, as I'm not as familiar with how projectData
gets passed around. I feel confident that this logic won't break anything because if L548 is null it'll just move on to the next line. But still, please confirm I have the right understanding!
For this screen, projectData.backing() = null (so L547 will not be used). Instead, the backing exists on the projectData.project(). And I want to get the reward off the backing because that's where the rewardImage
field lies, so that's what L548 does.
What I don't want to do is L549, where I try to find the reward off the project using the backedReward
extension function, because the reward off the project specifically does NOT contain the rewardImage
field:
fun Backing.backedReward(project: Project): Reward? {
val rewards = project.rewards() ?: return null
for (reward in rewards) {
if (isBacked(reward)) {
return reward
}
}
return null
}
I added tests to BackingFragmentViewModelTest to test L548 and L549 when the backing has a reward on it vs when the backing only has a rewardId on it.
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.
Fyi, take a look here for how ProjectData goes to BackingFragment ->
1-
android-oss/app/src/main/java/com/kickstarter/ui/activities/ProjectPageActivity.kt
Line 1039 in ea6ccdd
backingFragment.configureWith(projectData) |
2 -
android-oss/app/src/main/java/com/kickstarter/ui/activities/ProjectPageActivity.kt
Line 1245 in ea6ccdd
private fun updateFragments(projectData: ProjectData) { |
3 ->
android-oss/app/src/main/java/com/kickstarter/ui/activities/ProjectPageActivity.kt
Line 247 in ea6ccdd
this.viewModel.outputs.projectData() |
app:layout_constraintDimensionRatio="@string/reward_card_image_aspect_ratio" | ||
app:layout_constraintStart_toStartOf="parent" | ||
app:layout_constraintEnd_toEndOf="parent" | ||
app:layout_constraintTop_toTopOf="parent" /> |
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.
Can ignore the changes in this file after this line!
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2224 +/- ##
============================================
- Coverage 67.97% 67.93% -0.05%
Complexity 2197 2197
============================================
Files 356 356
Lines 23966 23983 +17
Branches 3521 3524 +3
============================================
Hits 16292 16292
- Misses 5829 5846 +17
Partials 1845 1845 ☔ View full report in Codecov by Sentry. |
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.
🎆 🥳 🎈 Finally!! getting Images on rewards!
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.
this looks so good! great job 🎉
📲 What
We're adding images on rewards!
🤔 Why
Because it's about damn time.
🛠 How
- Images on 3 different surfaces
Used Coil for image loading and caching. Rewards carousel, add ons, and manage pledge were all separate surfaces using different technologies (xml vs compose, vertical vs horizontal scrollview) that each needed to be dealt with differently. See the associated files with each screen to make your code review easier:
- API
Most of the API integration was done in #2219 but for the manage pledge screen I missed some graphql changes for the backing fragment, so added that to this PR. We can't simply add a field for image on the main Reward fragment because we want to fetch a bare minimum Reward object when querying Projects to prevent
transactionTooLarge
exception: #2173- Image crop
Images provided by server were not cropped! Image views had to be set to aspect ratio of 3:2 on client.
👀 See
📋 QA
Story 📖
https://kickstarter.atlassian.net/browse/MBL-2069