[Reader] Implement "Tags" feed horizontal posts list item component loading #20634
Conversation
|
 |
App Name |  Jetpack |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20634-c54757c | |
| Commit | c54757c | |
| Direct Download | jetpack-prototype-build-pr20634-c54757c.apk |
|
 |
App Name |  WordPress |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20634-c54757c | |
| Commit | c54757c | |
| Direct Download | wordpress-prototype-build-pr20634-c54757c.apk |
…t' into issue/20268-implement-horizontal-post-list-ui-shimmer
…t' into issue/20268-implement-horizontal-post-list-ui-shimmer
RenanLukas
changed the title
| isLoadingCompleted = false, | ||
| ) | ||
| } | ||
| ShimmerBox( |
There was a problem hiding this comment.
Maybe nitpick - I see creation of ShimmerBox instances with similar modifiers. Is there a way to further abstract these calls?
There was a problem hiding this comment.
@daniloercoli I think the color and shape repeats in many cases. The shape is not always the same, though, so maybe I can move the color to the ShimmerBox component? WDYT?
(Edit)
I've extracted the shimmer background color to the generic component.
There was a problem hiding this comment.
👍 Feel free to merge this PR.
…t' into issue/20268-implement-horizontal-post-list-ui-shimmer
There was a problem hiding this comment.
Nice. Looks cool so far. The shimmer color looks a bit too vibrant. Maybe it could be made more subtle, particularly on dark mode. I am not sure if the shimmer is a color or white/black with opacity. but if its white/black make it more transparent. If its a flat color, make it a color closer to the grey boxes.
|
@osullivanchris @daniloercoli Sorry, but I had to change the implementation of the shimmer. Could you please review it again? @osullivanchris I saw your comment about the shimmer color, but since I had to change the implementation I decided to show you how it looks right now before changing the colors. Here are the updated videos: Screen.Recording.2024-04-17.at.3.04.42.PM.movScreen.Recording.2024-04-17.at.3.04.28.PM.mov |
|
|
||
| background( | ||
| brush = Brush.linearGradient( | ||
| colors = listOf( |
There was a problem hiding this comment.
If i got it correctly we're using hard-coded colours. Does it work fine with all theme settings (light/dark mode)?
Should we use a theme-appropriate colours? Or add the ability to color values to be passed as parameters ?
| ) | ||
| ) | ||
| .onGloballyPositioned { | ||
| size = it.size |
There was a problem hiding this comment.
Should we check if the size has changed to prevent unnecessary recompositions? something like
.onGloballyPositioned { layoutCoordinates ->
val newSize = layoutCoordinates.size
if (size != newSize) {
size = newSize
}
}
| ) | ||
| } | ||
|
|
||
| @Suppress("MagicNumber") |
There was a problem hiding this comment.
Not sure it's worth the effort, but we can introduce 2 new constants and avoid the MagicNumber annotation?
const val SHIMMER_START_MULTIPLIER = -2
const val SHIMMER_END_MULTIPLIER = 2
|
@RenanLukas it still looks a bit too intense to me. I tried a quick prototype. What worked for me was:
Not sure how feasible this is or how much control you have. By the way, this could apply to any screen in our app. shimmer.mov |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/tags-ia #20634 +/- ##
===================================================
- Coverage 40.33% 40.15% -0.18%
===================================================
Files 1474 1465 -9
Lines 67878 67522 -356
Branches 11225 11183 -42
===================================================
- Hits 27377 27114 -263
+ Misses 38034 37959 -75
+ Partials 2467 2449 -18 ☔ View full report in Codecov by Sentry. |
|
As discussed in DM, the shimmer effect won't be implemented in this PR and will be taken care in #20679 This PR only implements the loading (grey rectangles) without shimmer effect. |
RenanLukas
deleted the
issue/20268-implement-horizontal-post-list-ui-shimmer
branch
Warning
Should be merged after #20614
Fixes #20628
Screen.Recording.2024-04-15.at.7.01.45.PM.mov
Screen.Recording.2024-04-15.at.7.01.56.PM.mov
To Test:
The UI component is not being used yet, so in order to test it you can run
HorizontalPostListItemLoadingPreviewand compare with Figma specs.Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
--
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):