Skip to content

DON-1396 Add Loading Label to BpkCalendarDayCell, update BpkSkeleton #2274

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

Merged
merged 9 commits into from
Apr 29, 2025

Conversation

henrik-sky
Copy link
Contributor

@henrik-sky henrik-sky commented Apr 25, 2025

This adds a new form of Label in the BpkCalendar, Loading.

As this is the smallest BpkSkeleton in use the BpkShimmerOverlay looked odd so the designers cooked up something new for small shimmers.
Therefore I added an enum to allow the consumer to set the size of shimmering animation.

I have also wrapped the Calendar label with AnimatedContent allowing for a smooth transition between the loading and the text label/icon.
The transition is delayed by 2x the length of the shimmer animation (design requirement).

Further documentation around our decision making for this can be found here:

Design:

Screen.Recording.2025-04-03.at.15.31.49.mov

Built:

BpkCalendarLoading.mp4

Remember to include the following changes:

  • Component README.md
  • Tests

If you are curious about how we review, please read through the code review guidelines

…erlay to support the smaller item's animation
@henrik-sky henrik-sky added the minor A new & backwards compatible feature/component label Apr 25, 2025
Update Loading cell to match design.
@henrik-sky henrik-sky force-pushed the donburi/DON-1396_Calendar_Loading branch from 94b2927 to 7b23486 Compare April 25, 2025 14:49
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@peterInTown peterInTown left a comment

Choose a reason for hiding this comment

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

LGTM

@henrik-sky henrik-sky marked this pull request as ready for review April 29, 2025 10:59
@henrik-sky henrik-sky force-pushed the donburi/DON-1396_Calendar_Loading branch from d8cf33b to 759793a Compare April 29, 2025 15:14
@@ -158,6 +158,8 @@ sealed class CellLabel : Serializable {
val resId: Int,
val tint: Int? = null,
) : CellLabel()

data class Loading(val contentDescription: String) : CellLabel()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should replace serialisables with Parcelables/custom savers when we can. Not as a part of this PR though

@@ -102,6 +102,13 @@ enum class BpkSkeletonCornerType {
Rounded,
}

enum class BpkShimmerSize(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be really called size? It looks more like timings for me. Is that the naming we use on Figma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the timings for the size of skeleton

shimmerSize: BpkShimmerSize = BpkShimmerSize.Large,
content: @Composable BoxScope.() -> Unit,
) {
BoxWithConstraints(modifier = modifier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

BoxWithConstraints will have performance implications, especially when used in lists (which we do in many places). How about this: we'll animate the float "fraction/percent" value, then simply multiply it to the width that's available in graphicsLayer {} modifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I think I had something similar previously but got confused with the typeConverter when making the animation but then today I realised it's just Float.VectorConverter so now it goes from

-1f * GraphicsLayerScope.size.width to 1f * GraphicsLayerScope.size.width

🚀

Copy link
Contributor

@bvitaliyg bvitaliyg left a comment

Choose a reason for hiding this comment

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

A couple of small comments

Copy link
Contributor

@bvitaliyg bvitaliyg left a comment

Choose a reason for hiding this comment

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

Approved, but please make the timings internal for the size

@henrik-sky henrik-sky enabled auto-merge (squash) April 29, 2025 16:09
@henrik-sky henrik-sky merged commit f69b30f into main Apr 29, 2025
7 checks passed
@henrik-sky henrik-sky deleted the donburi/DON-1396_Calendar_Loading branch April 29, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor A new & backwards compatible feature/component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants