Skip to content

Conversation

@ef4
Copy link
Contributor

@ef4 ef4 commented May 9, 2019

Closes #105.

This is deliberately grouped into multiple commits with different purposes to ease review.

  • the image URL handling was fragile, such that this component only renders correctly on the index route and nowhere else (meaning it didn't render correctly in integration tests)
  • added integration tests for the existing behaviors
  • added a failing test for the interruption case that animated poorly
  • fixed it

ef4 added 4 commits May 9, 2019 11:52
The demo-3 component only works when used on the index route. I noticed because I started trying to write an integration test for it, and it couldn't render its images correctly there.
@ef4 ef4 requested a review from samselikoff May 9, 2019 16:44
@samselikoff
Copy link
Contributor

I took some time to look through these commits, to try to understand both how to test animations and what this particular fix entailed.

First – I'm blown away by how you're able to test such a complex animation. Truly! This is an amazing aspect of this lib that deserves an entire section and multiple guides in the docs.

Second, for this particular bug, I'd like to check my understanding. Let's just focus on collapse:

  • My original implementation of collapse naively only animated receivedSprites, since I elected to animate the shuffling side of the animation using the other animated-each animator that wrapped the larger images
  • Even though I chose not to animated sentSprites, the sprites are still in that category when the condition ((if (not-eq selectedCategoryName category.name) category.images)) flips and the array being each'd over flips from [cat1, cat2, cat3] to [])
  • On interruption, the receivedSprites (which I actually care about) are getting their initialBounds from... where? Are the receivedSprites being affected by the sentSprites of the previous run of the transition? And why does moveToFinalPosition fix the problem?

Amazing PR and thank you so much for taking the time to fix & write such clear code!

@ef4
Copy link
Contributor Author

ef4 commented May 29, 2019

I sat down to try to answer this question and realized I don't actually have the fully detailed answer. I need to run the code again to see it in more detail. 🤪

@samselikoff
Copy link
Contributor

@ef4 ping

@samselikoff
Copy link
Contributor

I can fix the conflicts but still curious on the explanation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Documentation example problemo

3 participants