Leave does't take into account scroll position about hyperapp-transitions HOT 14 CLOSED

Haha yeah it's pretty dense, but I've been able to make sense of it so far!

from hyperapp-transitions.

@snalld

I'll have a look into move and get a codepen going asap.

If it's not too much trouble, that would be very helpful to solving this in the best way!

Hadn't thought about scrolled divs... I'm guessing you'd need to step through any scrolled parent nodes and and account for the scroll pos of each..

...yeesh... but yeah, you're right. Perhaps I could use offsetParent instead of clientBoundingRect(). That way we'd always be calculating the movement in relation to the immediate parent.

The viewport thing gave me an idea for an alternative solution. What if we, instead of keeping track of scroll-position in trackMoves, were to simply add an eventlistener to track movement also on scroll-events and window resize events?

My thinking is, the problem occurs because the element moves in relation to the viewport without our "knowledge", because trackMoves only occurs for onupdate. But if we could track movement of the element every time it moves in relation to the viewport, regardless of why, the problem would go away.

I'd start tinkering with it right away if I weren't so swamped atm. Feel free to look into it if you like, as it will likely be at least few days before I can really dig into it properly. I only mention it so you don't think I don't care if I go silent for a bit 😉

from hyperapp-transitions.

@snalld glad to hear that solution works! Going to work on it tonight.

What I'll probably do is listen to 'resize' and 'scroll' events just once, and make updateAttrs basically go through a list of elements we're currently tracking position on. That should probably address your performance concerns and will help with the complication of un-listening to the events as well.

I've also been able to verify the scroll-position bug myself (using the "Toasts" example). So I have a way to verify the solution.

from hyperapp-transitions.

@zaceno no worries!

I'm happy with not having a tracking flag there, although it's comforting to know you can opt out...

Re performance, is it worth adding requestAnimationFrame to help throttle updates when there's heaps of tracked elements?? Tbh I can think of when I'd have enough tracked elements to make a serious impact on performance

I'm also having chrome throw errors with onremove (haven't had a look in other browsers though...)

Looking forward to the next version!

from hyperapp-transitions.

Thanks for pointing this out @snalld I'll look into it!

from hyperapp-transitions.

Haven't tested it beyond my immediate use-case but seems all I needed to was tweak trackMoves

function trackMoves (el) {
    const prevX = +el.getAttribute('data-t-x')
    const prevY = +el.getAttribute('data-t-y')
    const {left: newX, top: newY} = el.getBoundingClientRect()
    el.setAttribute('data-t-x', newX)
    el.setAttribute('data-t-y', newY)

    // Added to account for change in scroll position
    const prevScrollX = +el.getAttribute('data-s-x')
    const prevScrollY = +el.getAttribute('data-s-y')
    const newScrollX = window.pageXOffset
    const newScrollY = window.pageYOffset
    el.setAttribute('data-s-x', newScrollX)
    el.setAttribute('data-s-y', newScrollY)

    if (!prevX) return [null, null]

    // Updated to accound for change in scroll position
    return [(prevX - newX) + (prevScrollX - newScrollX), (prevY - newY) + (prevScrollY - newScrollY)]
}

Thoughts?

from hyperapp-transitions.

Lol also sorry for not just forking dunno why i did it this way round

from hyperapp-transitions.

That looks good to me!

Have you by any chance checked if this is also a problem, and that your fix fixes (or at least doesn't break) for move transitions when scrolling? (Oh if possible, a codepen to demo the problem would be super)

You're welcome to fork & PR if you like! (If not, I'll probably just take your code as you pasted above, but I want to look into it a little more first. Curious about how this affects move transitions, and also if this fix will work for elements in scrolled divs on scrolled pages ... if that's even a use case)

from hyperapp-transitions.

I'll have a look into move and get a codepen going asap.

Hadn't thought about scrolled divs... I'm guessing you'd need to step through any scrolled parent nodes and and account for the scroll pos of each..

Also, just found that for a very similar reason resizing the viewport causes the same issue. Only problem is that the element transform would need to be recalculated on every resize.. Not sure how to do this in a nice way?

from hyperapp-transitions.

The viewport thing gave me an idea for an alternative solution. What if we, instead of keeping track of scroll-position in trackMoves, were to simply add an eventlistener to track movement also on scroll-events and window resize events?

I was thinking event listener as well. Would you have a collection for tracked elements that gets iterated over on each change? What's the best way to go about this? If you give me a lead I'll look into while you're swamped!

from hyperapp-transitions.

Hmm, no collections, sorry. I was thinking we could perhaps just register event listeners when the element we want to track is created (when the first trackMoves for an element is called). Like here, for leave-transitioning elements:

Does that make sense do you think? (...haven't thought ahead enough to consider when we remove them, though)

Sorry the code is so convoluted.... once I realized I had to add the position-tracking stuff, the code got pretty big and I went on a raid to try to remove all repetition. I'm super glad you're willing to dig into it, but feel a little bad it's so impenetrable now 😜 . If you have any nice ideas for making it more readable and easy to understand, be my guest 😄

from hyperapp-transitions.

Haven't really tested this but it seems to fix the scroll and resize issue...

function trackMoves(el) {

  addEventListener('resize', () => {
    updateAttrs(el)
  }, false)

  addEventListener('scroll', () => {
    updateAttrs(el)
  }, false)

}

function updateAttrs(el) {
  const prevX = +el.getAttribute('data-t-x')
  const prevY = +el.getAttribute('data-t-y')
  const { left: newX, top: newY } = el.getBoundingClientRect()
  el.setAttribute('data-t-x', newX)
  el.setAttribute('data-t-y', newY)

  if (!prevX) return [null, null]

  return [(prevX - newX), (prevY - newY)]
}

Wouldn't advise using it on many elements though. Performance would suffer. Also I've tweaked the _leaveOnCreate _leaveOnRemove functions to only track movement if you ask them to (using an additional prop props.track)

from hyperapp-transitions.

Generally with this library I'm noticing some misbehavior in chrome which I'm not seeing with safari. Since hyperapp-transitions hasn't changed, it must be due to recent changes in hyperapp (I suspect something to do with the repaint-scheduling). So I'm probably looking at a ground up rewrite soon. Hopefully that means I can simplify things a bit too.

from hyperapp-transitions.

@snalld thanks for your help and patience. v0.6.7 is shipped and afaik fixes the issue. Please let me know if this works for you as well!

I decided not to include the track flag to turn on/off position tracking for two reasons:
a) things are complicated enough already ;) and
b) position tracking only becomes a performance issue when there are a lot of elements moving or leaving -- which is exactly the situation when you need position tracking anyway.

... I may be wrong about (b) though, so feel free to correct me :)

from hyperapp-transitions.