Discover message card design #5590
Conversation
- code/ui fixes - adds compose layout on ReadingListsFragment.kt - adds a function get 4 latest images for recommended reading list
- instead of retrieving images url now gets the new recommended articles
- adds click action on discover card view
app/src/main/java/org/wikipedia/readinglist/DiscoverReadingListView.kt
Outdated
Show resolved
Hide resolved
| ) { | ||
| Column( | ||
| modifier = Modifier | ||
| .weight(1.1f), |
There was a problem hiding this comment.
Not sure but should this be a fixed dimension based on the design?
There was a problem hiding this comment.
This weight is for horizontal distribution. I'm not using a 1 to 1 because it makes the image slightly wider, which doesn’t look visually good.
| LazyVerticalGrid( | ||
| modifier = Modifier | ||
| .clip(RoundedCornerShape(12.dp)) | ||
| .weight(0.9f), |
There was a problem hiding this comment.
Same question about the fixed dimension here (based on design dimensions or is it OK to use weights)
There was a problem hiding this comment.
yes it is okay to use weights.
| placeholder = ColorPainter(WikipediaTheme.colors.borderColor), | ||
| error = ColorPainter(WikipediaTheme.colors.borderColor), |
There was a problem hiding this comment.
Hi @Williamrai This looks like a fallback which is a great idea. I was wondering if the color was ok to use? I couldnt find an example of a placeholder/error color.
There was a problem hiding this comment.
I'm using the default color we typically use for placeholders, and I'm also applying the same color for errors. You can refer to the loadImage function in the CoilImageServiceLoader class.
| @@ -60,6 +63,23 @@ fun Modifier.pulse( | |||
| } | |||
| } | |||
|
|
|||
| fun Modifier.rippleClickable( | |||
There was a problem hiding this comment.
Hi @Williamrai I like this extension function! It definitely provides consistency for ripple behavior when clicking. One thought I had was the usage of the function. If we plan to use it a lot then its definitely worth keeping but if it sees limited use then perhaps it make may be easier to use modifier.clickable() and set the parameters there?
This isn't a request to change anything right now. Overall, it’s a good idea and a solid addition!
There was a problem hiding this comment.
A similar function was created by Cooltey in one of his PRs, so once the PR is merged, we'll use one of the two implementations.
| fun DiscoverReadingListView( | ||
| modifier: Modifier = Modifier, | ||
| title: String, | ||
| @DrawableRes subtitleIcon: Int, |
There was a problem hiding this comment.
It may be worth considering hardcoding the drawable into the Icon composable since the DiscoverReadingListView is a specialized composable for this feature.
There was a problem hiding this comment.
Yes, we can do that, but I believe it's helpful to keep this as a parameter. This way, the icon can be modified directly from the caller without needing to dive into the implementation. It also allows flexibility in the future if we decide to use different icons for different scenarios.
There was a problem hiding this comment.
Hi @Williamrai I only reviewed the jetpack compose code but please let me know if you have any questions.
@voyagerfan Thank you for your review. |
…card-design # Conflicts: # app/src/main/java/org/wikipedia/readinglist/ReadingListsFragment.kt
…card-design # Conflicts: # app/src/main/java/org/wikipedia/readinglist/ReadingListsFragment.kt
What does this do?
Phabricator:
https://phabricator.wikimedia.org/T394055