search

choojs / nanocomponent

🚃 - create performant HTML components
https://leaflet.choo.io/
MIT License
366 stars 30 forks source link

Run afterreorder immediately #58

Closed tornqvist closed 7 years ago

tornqvist commented 7 years ago

Offsetting afterreorder to next animation frame makes it difficult to do any kind of animation in reaction to reordering, seeing as the new order will already have been repainted by the browser before the callback is fired.

Chrome and Firefox handles it quite nicely offsetting the repaint two frames so that the animation runs smoothly but Safari seems to perform an intermediate paint between the requestAnimationFrame in nanocomponent and the requestAnimationFrame in my script which is needed to make the animation happen, (see source: https://github.com/tornqvist/fun-component/blob/master/examples/list/index.js#L32)

Here's an example illustrating the issue: https://fun-component-list.now.sh

Am I missing an apparent reason for offsetting the callback? There are some other issues, e.g. not all elements are considered being reordered but just switch places, this is also visible in the example. But I think removing the requestAnimationFrame fixes the most pressing issue.

bcomnes commented 7 years ago

Good question. There are some nuances around the on-load hooks that I can't recall off the top of my head, and some of the code in nc existed before I had my hands on it, but this could be a reasonable change. I'll try to look into it asap

bcomnes commented 7 years ago

Sorry @tornqvist I will get to this by this evening.

bcomnes commented 7 years ago

@tornqvist curious if this fixes the animation or if thats a side effect of how we sibling reorder

tornqvist commented 7 years ago

Hey, thanks for the quick fix. It does solve the problem with Safari rendering the intermediate list order but some item's afterreorder callback still isn't being called. See: https://fun-component-list.now.sh

Also, you should know that my tests started failing with this change. I imagine it's an edge case to be removing an element on afterupdate but doing so (w/o requesting an animation frame) would cause the on-load tree walker to throw an error when trying to access childNodes of the newly removed node. I solved it by offsetting the removal by an animation frame (see: https://github.com/tornqvist/fun-component/commit/1b775635caeb5aff787b95a326bcfe098d73733e#diff-77686aae68ae4b50e2efc518d77fd6d3L79)

bcomnes commented 7 years ago

Strange! We didn't touch afterupdate, but perhaps the removal of the async context in the load and unload events you lost an async context that is needed there. See https://github.com/choojs/nanocomponent/pull/59/files for the total list of changes that were made yesterday.

Generally removing nodes from lifecycle hooks should be done in an async context, but I am still figuring out the implications of it all myself 😅

The reorder event gets fired when Dom nodes are removed from the page and then re-added, so not all of the nodes end up getting removed during nanomorph sibling re-ordering, only some. If you wanted to guarantee animation of all rows, it might not be the right hook (although maybe its possible? I haven't really thought through that problem.). I added afterreorder for iframes which have issues with being unmounted for a moment, so you can reinitialize them, and other possible edge cases related to temporary removal from the DOM.

Open to ideas about how to improve this situation.

tornqvist commented 7 years ago

Kewl. That makes sense. I guess a better approach would be to supply an index on render and handle the animation on afterupdate using the index to figure out where to animate from. I was just interested in seeing what use cases there were for afterreorder. Thanks for the feedback and the hard work!

bcomnes commented 7 years ago

The reordering still has a few issues. See https://nanomap.netlify.com. Shifting items out of the list should just remove the first child DOM node, but instead triggers a worse case morph somehow.

@yoshuawuyts says we need something like a Levenstein algorithm for reordering to get the best possible reorder, as right now its a bit haphazard in some cases.

Anyway, you are probably right that relying on side effects of morph behavior might not be the best approach as it could change out underneath you! Perhaps a list component that tracks re-order index before and after reorder would be the most deterministic approach.

yoshuawuyts commented 7 years ago

Yeah, I hope to get a patch out for reordering this week. Think we got a good sense on how to do it now (:

On Wed, Sep 20, 2017 at 6:07 PM Bret Comnes notifications@github.com wrote:

The reordering still has a few issues. See https://nanomap.netlify.com. Shifting items out of the list should just remove the first child DOM node, but instead triggers a worse case morph somehow.

@yoshuawuyts https://github.com/yoshuawuyts says we need something like a Levenstein algorithm for reordering to get the best possible reorder, as right now its a bit haphazard in some cases.

Anyway, you are probably right that relying on side effects of morph behavior might not be the best approach as it could change out underneath you! Perhaps a list component that tracks re-order index before and after reorder would be the most deterministic approach.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/choojs/nanocomponent/pull/58#issuecomment-330901196, or mute the thread https://github.com/notifications/unsubscribe-auth/ACWlejwHB4U7pg3l-puEw7oA9X2CSRAgks5skTgzgaJpZM4PVrbd .