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

Animating article items being dismissed #688

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

savvasdalkitsis
Copy link

This will make items being swiped away animate the rest of the list so there is no jarring jump when the article is removed

@nvllz
Copy link
Contributor

nvllz commented Apr 18, 2024

Whoa! Never thought it's that easy.. Works fine.

Could you also apply this to the publication date headers? In the case shown below these are not as smooth.

00dc4867-6d58-4470-9407-2749b257a5ee.mp4

Thanks, @savvasdalkitsis!

@Ashinch
Copy link
Owner

Ashinch commented Apr 19, 2024

This looks great.

@Ashinch Ashinch added this to the 0.10.0 milestone Apr 19, 2024
@Ashinch Ashinch added the enhancement New feature or request label Apr 19, 2024
Copy link
Owner

@Ashinch Ashinch left a comment

Choose a reason for hiding this comment

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

LGTM

@savvasdalkitsis
Copy link
Author

I pushed another change to animate the headers

@JunkFood02
Copy link
Collaborator

@savvasdalkitsis new release of compose animation has introduced addition/deletion animation for list items animateItem(), you can give it a try

@nvllz
Copy link
Contributor

nvllz commented Apr 22, 2024

Unfortunately I noticed that the animation tends to cause blank gaps on dismiss.

85f9eec5-670b-47e0-8ee4-19d82f1f0a3f.mp4

It's kinda random, sometimes it works well.

@savvasdalkitsis
Copy link
Author

@nvllz i cannot replicate it locally but i updated to the latest compose bom in case there is a bug fix in there that addresses this. can you try it and see if the issue shows up again?

@nvllz
Copy link
Contributor

nvllz commented May 6, 2024

It's the same, ugh. Does it really work as intended for you after you enter an article, go back to the flow page, and swipe out some articles? The animation works fine until I enter the reader view.

d2a0ed64-af45-4328-a02a-e45fbc5493c7.mp4

My Flow Page settings:
bfd0bb22-a214-47a5-9738-b41642f81a9d

@savvasdalkitsis
Copy link
Author

@nvllz I believe updating to the alpha/beta versions of compose and using the new animateItem method that @JunkFood02 has mentioned fixes the issue.

The question now is if you are comfortable moving to a non stable version of compose. If not, we may have to wait until the method makes it to stable.

Also note that the last commit also introduces a weird interaction in the swipe box that the remove animation runs after the swipe box snaps back to the original position. Not sure if that would be a blocker for you

@nvllz
Copy link
Contributor

nvllz commented May 6, 2024

Just tested the latest build and can confirm that article swiping seems to be fixed. But this kinda broke the sticky headers feature, as turning them on causes the flow page to throttle when scrolling.

Also note that the last commit also introduces a weird interaction in the swipe box that the remove animation runs after the swipe box snaps back to the original position. Not sure if that would be a blocker for you

Not sure what you mean. I just noticed that the article background is not colored when swiping, which looks even better now.

@savvasdalkitsis
Copy link
Author

I removed the animating of the stickyHeader to see if that fixes the performance issue you mentioned @nvllz (it looked the same to me on the emulator)

For the swipe issue i mentioned, see the video below:

Screen_recording_20240507_084322.mp4

Previously, when you swiped an item away, it wouldn't bounce back and then dissapear, it would dissapear immediately. hope that makes sense

@nvllz
Copy link
Contributor

nvllz commented May 7, 2024

It doesn't bounce back for me, tested with Galaxy S20+.

9e3e38e0-c043-4ac0-9f5d-ea858c557d18.mp4

@savvasdalkitsis
Copy link
Author

Well, that's weird 😅

@nvllz
Copy link
Contributor

nvllz commented May 7, 2024

I removed the animating of the stickyHeader to see if that fixes the performance issue you mentioned @nvllz (it looked the same to me on the emulator)

Unfortunately it doesn't fix the problem, as the flow page keeps lagging with sticky headers enabled. But I figured out how to trigger this bounce back animation, using a longer swipe.

335fd50f-6944-476e-937c-2ea063b5d7cc.mp4

(Sticky headers disabled)

@Ashinch Ashinch modified the milestones: 0.10.0, 0.11.0 Jun 2, 2024
@Ashinch
Copy link
Owner

Ashinch commented Jun 19, 2024

Hi everyone, what’s the current status of this PR? I mean, can it be included in the next release?

@Ashinch Ashinch added the RFC Request for Comments label Jun 19, 2024
@JunkFood02
Copy link
Collaborator

blocked by #772

@nvllz
Copy link
Contributor

nvllz commented Jun 19, 2024

I used a build with this PR included and it worked well. The only imperfection was the card sliding back animation on a long swipe, shown above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request RFC Request for Comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants