Skip to content
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

[Woo POS] Design: Products Screen animation #12557

Merged
merged 22 commits into from
Sep 21, 2024

Conversation

backwardstruck
Copy link
Contributor

@backwardstruck backwardstruck commented Sep 6, 2024

Closes: #12556

Description

This PR addresses a design concern on the Products Screen as outlined in issue #12556:

Item Addition Animation: Implemented a smooth slide-down animation for newly added products. This change makes the addition of items at the top more intuitive and visually pleasing, resolving the sudden shift previously observed.

Steps to reproduce

  1. Navigate to POS.
  2. Add a new product to the cart.
  3. Add another product
  4. Add more products
  5. Verify that the new product slides in smoothly from the top, and the existing products move down gently.

Testing information

Tested on:

  • Devices: tablet emulator and physical Samsung tablet
  • Checked the smooth addition of new products and verified the animations are functioning as intended.

Videos

Dark mode

darkmode.mp4

Light mode

lightmode.mp4

The tests that have been performed

  • Added multiple products and observed the animation.

  • Removed products and ensured the layout adjusts without issues.

  • Tested on different screen sizes to ensure consistent behavior.

  • Checked for regressions or performance issues.

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@backwardstruck backwardstruck added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 6, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 6, 2024

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit98a2533
Direct Downloadwoocommerce-wear-prototype-build-pr12557-98a2533.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 6, 2024

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit98a2533
Direct Downloadwoocommerce-prototype-build-pr12557-98a2533.apk

@backwardstruck backwardstruck added this to the 20.4 milestone Sep 9, 2024
animationSpec = tween(durationMillis = 200, delayMillis = 100),
label = "elevation"
)
AnimatedVisibility(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this suggestion Michał.

@backwardstruck backwardstruck marked this pull request as ready for review September 9, 2024 14:48
@backwardstruck backwardstruck added feature: point of sale POS project and removed status: do not merge Dependent on another PR, ready for review but not ready for merge. labels Sep 9, 2024
@backwardstruck backwardstruck changed the title [Woo POS] Design: Products Screen changes [Woo POS] Design: Products Screen animation Sep 9, 2024
…reen-changes' into 12556-woo-pos-design-products-screen-changes
@kidinov kidinov self-assigned this Sep 10, 2024
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

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

@backwardstruck 👋

A few notes:

  • You are animating the Compose that has appearance animation already in place, so double animation
  • You use state declared in the compose itself, so it'll animate everytime when it's recreated. Try to scroll the items in the cart - it will animate it everytime when a "view" reused. Notice that there is already state stored in the VM isAppearanceAnimationPlayed used for exactly this animation

Having said that, considering that it looks like in the Compose there is a bug/missing feature that shadows are gone when element is in animation. That's why I was trying to smooth this app with elevation being animated too, but it looks pretty bad anyway.

I'd advice asking Joe what looks better - no animation at all or animation how we can implement it in the moment

@backwardstruck
Copy link
Contributor Author

@backwardstruck 👋

A few notes:

  • You are animating the Compose that has appearance animation already in place, so double animation
  • You use state declared in the compose itself, so it'll animate everytime when it's recreated. Try to scroll the items in the cart - it will animate it everytime when a "view" reused. Notice that there is already state stored in the VM isAppearanceAnimationPlayed used for exactly this animation

Having said that, considering that it looks like in the Compose there is a bug/missing feature that shadows are gone when element is in animation. That's why I was trying to smooth this app with elevation being animated too, but it looks pretty bad anyway.

I'd advice asking Joe what looks better - no animation at all or animation how we can implement it in the moment

Thanks for the info @kidinov Just to confirm, you are referring to the existing elevation and alpha animations as causing the double animation, right? If so, I agree that we can't have those if I am also adding a new animation.

I will use VM state, like isAppearanceAnimationPlayed going forward to ensure that my animations behave consistently.

# Conflicts:
#	WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/cart/WooPosCartScreen.kt
@backwardstruck backwardstruck added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 10, 2024
@kidinov
Copy link
Contributor

kidinov commented Sep 11, 2024

@backwardstruck

Just to confirm, you are referring to the existing elevation and alpha animations as causing the double animation, right? If so, I agree that we can't have those if I am also adding a new animation.

Yep!

The question is, what will be the difference, though? My understanding is the existent animations are close/the same as what you are adding with animated visibility, and the current ones aren't good.

So we either improve those (as mentioned, the main issue is that the shadows are gone during animation) or maybe we remove the animation. I am not sure what's better

# Conflicts:
#	WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/cart/WooPosCartScreen.kt
@backwardstruck
Copy link
Contributor Author

@kidinov I checked again against trunk and now I think it may be OK to add my animation since it mainly affects how the existing items in the list are animated.

Here's what I see in testing:

Trunk

trunk.mp4

This PR

PR1.mp4

Indeed, the elevation and alpha animations already in trunk are a bit harder to see in my PR because it's also sliding in the item from the top. I agree we should check with @joe-keenan to see if any of these animations are what's actually needed.

@backwardstruck backwardstruck removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Sep 11, 2024
@backwardstruck backwardstruck added the category: design Layout and style elements in the UI or user interface, including color and animations. label Sep 11, 2024
@backwardstruck backwardstruck modified the milestones: 20.4, 20.5 Sep 11, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 40.64%. Comparing base (f7a50f4) to head (98a2533).
Report is 23 commits behind head on trunk.

Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #12557      +/-   ##
============================================
- Coverage     40.64%   40.64%   -0.01%     
+ Complexity     5675     5674       -1     
============================================
  Files          1229     1229              
  Lines         69178    69178              
  Branches       9579     9579              
============================================
- Hits          28120    28119       -1     
  Misses        38475    38475              
- Partials       2583     2584       +1     
Flag Coverage Δ
40.64% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joe-keenan
Copy link

Looks pretty good, just a couple things: The animation is a little slow, and the shadow still appears last, after the card has faded in.

Could you speed it up, and have the shadow fade in with the card?

@kidinov
Copy link
Contributor

kidinov commented Sep 16, 2024

@backwardstruck 👋

fadeIn and fadeOut animations are the animations that change alpha. So you have 2 animations now that do the same. So probably

    val alpha by animateFloatAsState(
        targetValue = if (hasAnimationStarted) 1f else 0f,
        animationSpec = tween(
            durationMillis = 200,
            easing = LinearEasing
        ),
        label = "alpha"
    )

should be removed

@backwardstruck
Copy link
Contributor Author

backwardstruck commented Sep 19, 2024

Looks pretty good, just a couple things: The animation is a little slow, and the shadow still appears last, after the card has faded in.

Could you speed it up, and have the shadow fade in with the card?

Thanks for the feedback @joe-keenan I just pair programmed with @kidinov to speed up the animations. The shadow still appears last but it's not coinciding with an elevation change, which hopefully looks better. We can revisit this after the DM by potentially upgrading to a newer version of Jetpack Compose. That may include a fix for the shadow issue we are working around in this PR.

FYI: You can see the new animation in the video recordings at the top of this PR in the description. Let me know if you want us to make more changes in a follow up PR.

Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

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

LGTM! Left one np; feel free to merge

@backwardstruck backwardstruck modified the milestones: 20.5, 20.6 Sep 20, 2024
@backwardstruck backwardstruck merged commit 1212943 into trunk Sep 21, 2024
14 of 15 checks passed
@backwardstruck backwardstruck deleted the 12556-woo-pos-design-products-screen-changes branch September 21, 2024 00:39
@joe-keenan
Copy link

That is nicer. Too bad about the shadow, but I can live with it after those other improvements.

Dark mode looks a little odd, the way the background appears at the same time as the shadow, but it’s not a big stress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: design Layout and style elements in the UI or user interface, including color and animations. feature: point of sale POS project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Woo POS] Design: Products Screen changes
5 participants