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

Moving content breaks perimeter #9

Open
JulienPradet opened this Issue Mar 18, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@JulienPradet

JulienPradet commented Mar 18, 2017

Hi,

First of all, thanks for the lib. It's simple but smart!

However, one of my concerns is that when the content is moving, it actually breaks the perimeter since this.bounds is not updated. (I've checked the ongoing PRs and they dont seem to address this problem)
The resize window event is a first step but is not enough.

I originally thought about adding the componentDidUpdate lifecycle method but if the updated content is not in the component itself it won't solve the issue (see updated demo in my fork https://github.com/JulienPradet/react-perimeter).

One way, though, to address this problem would be to update bounding shapes using a requestIdleCallback (in order to avoid freezing important stuff going on) and debouncing it (since the mouse's movements fire a lot of events) directly. This would be doable in the provider that will land in @jnsdls 's PR. This solution allows people not to think about updating their shapes and keeps the API simple. However it might lead to performance issues since there would be ton of updates.

If people want a more manual way to update those, the automatic update could be disabled and the update method could be exposed in the context in order for them to call it in their own component lifecycles.

If what I'm talking about is not in the scope of the lib, feel free to ignore it.
Additionnally, if what I'm saying is not clear enough I could land a POC if you're interested in it.

👋

Edit: Oh, and thinking about it, the perimeter could check the hover event on the registered DOM node too! This way, even if the perimeter is broken, the preload would still launches on hover which is still nice to have. 🙂

@aweary

This comment has been minimized.

Owner

aweary commented Mar 18, 2017

Thanks for the issue @JulienPradet! You bring up a great point, current react-perimeter has no way of knowing if an element is moved outside a resize event. I think the majority of users are likely to use this with ~mostly static elements like navigation links or call to action buttons, but I'd still love to support this case.

One way, though, to address this problem would be to update bounding shapes using a requestIdleCallback (in order to avoid freezing important stuff going on) and debouncing it (since the mouse's movements fire a lot of events) directly. This would be doable in the provider that will land in @jnsdls 's PR. This solution allows people not to think about updating their shapes and keeps the API simple. However it might lead to performance issues since there would be ton of updates.

As far as I know, requestIdleCallback isn't the best place to do DOM work (reference). We could use a requestAnimationFrame loop, but we'd need to determine the element's position in the page which will force layout calculations and doing that frequently will potentially hurt performance.

If people want a more manual way to update those, the automatic update could be disabled and the update method could be exposed in the context in order for them to call it in their own component lifecycles.

I like the idea of letting users decide when the bounds should be recalculated, but that still seems difficult if some change in a separate part of the render tree is affecting the element's position. It could be difficult and tedious if you have a handful of perimeters drawn on the screen and you have to manually consider all cases where the layout changes.

That's a tough problem, I'm not totally sure what the best approach is yet!

Edit: Oh, and thinking about it, the perimeter could check the hover event on the registered DOM node too! This way, even if the perimeter is broken, the preload would still launches on hover which is still nice to have. 🙂

Great idea!

@JulienPradet

This comment has been minimized.

JulienPradet commented Mar 18, 2017

As far as I know, requestIdleCallback isn't the best place to do DOM work (reference). We could use a requestAnimationFrame loop, but we'd need to determine the element's position in the page which will force layout calculations and doing that frequently will potentially hurt performance.

Actually, the reason why I talked about requestIdleCallback was because the things handled by react-perimeter is background stuff. If it's slow to update, it is no big deal, since that's just a few optimizations for future navigation and not for current view. We're not actually manipulating the DOM, we're just retrieving its positions.

However, if we use requestAnimationFrame, the risk is that we would do updates while an actual animation is occuring somewhere else. If so, it means we will seriously injure the animation's snappyness. Indeed, since the coordinates are calculated using getBoundingClientRect (and it would be the same with any other method that retrieves the DOM's position), there would be layout thrashing which is like the worst enemy of animators :P

I'll get a demo out first in order to check what I'm saying. But I'd expect the event loop stack not to impact the layout/painting process.

<edit>Oh, you tweeted about it ^^</edit>

I like the idea of letting users decide when the bounds should be recalculated, but that still seems difficult if some change in a separate part of the render tree is affecting the element's position. It could be difficult and tedious if you have a handful of perimeters drawn on the screen and you have to manually consider all cases where the layout changes.

Yup, that's the curse of manual checking. I'll first try to land a PR with the automatic update thing mentioned above, if that's fine for you. Maybe things will be clearer in my head by then.

Finally, about the onHover thing, I'll land a seperate PR for it.

@JulienPradet

This comment has been minimized.

JulienPradet commented Mar 19, 2017

Here are 4 timelines demonstrating the difference between requestAnimationFrame and requestIdleCallback. The red overlay indicates when the mouse is moving (identified by the additional JS performing in the 'main' timeline). I've done these with two straight mouse moves. However the result is more or less the same with a continuous and circular mouse move. I just happen to have more random drops in my timeline.

without debounce

requestAnimationFrame:
requestanimationframe

requestIdleCallback:
requestidlecallback

with a 100ms debounce

requestAnimationFrame : (the drop at 2000ms is just a random one I guess)
debounceandrequestanimationframe

requestIdleCallback:
debounceandrequestidlecallback

Demo code can be found here https://github.com/JulienPradet/react-perimeter/blob/master/src/index.js#L116
Didn't looked much into cleaning the onBreach method. I've just moved around without thinking about code quality yet.

@yasserkaddour

This comment has been minimized.

yasserkaddour commented Jun 12, 2017

@aweary Thanks for this awesome lib, and thank you @JulienPradet for trying to fix this issue.

I wanted to use react-perimeterfor an infinite scroll page, but this issue is a blocker, because the perimeter is not moved to bottom of the page.

Would be great to hear any update on this. Thanks again ✌️

@JulienPradet

This comment has been minimized.

JulienPradet commented Jun 16, 2017

Hi @yasserkaddour,

I haven't gone any farther since I didn't know if my suggested solution would be merged (for lib size + lib goals reasons). Plus, there are two solutions that I can think of and didn't know which one to implement since having both might be useful.

@yasserkaddour

This comment has been minimized.

yasserkaddour commented Jun 16, 2017

I managed to make it work by re-rendering the perimeter after each payload. But it would be great if this was supported out of the box.

Julien, merci pour la contribution.

@aweary

This comment has been minimized.

Owner

aweary commented Jun 20, 2017

I'm really hesitant to implement a solution that involves constantly redrawing the boundaries (even if it's debounced). Ideally, there would be a way for the application to tell Perimeter that it needs to redraw its boundaries. Re-rendering implicitly does this, but that's not cheap.

We could:

  • Provide a prop that lets you opt-in to some automatic redraw strategy
  • Provide a prop like redrawInterval that lets users decide how frequently to redraw the boundaries (if at all).
  • Export some imperative redraw utility that can be called explicitly to redraw some set of boundaries.
import { Perimeter, redraw } from 'react-perimeter'
// Render a Perimeter with a name attribute
<Periemeter name="card" />
// Somewhere else, call redraw with the name
redraw("card");
// Or even call it without a name to redraw all perimeter instances
redraw();
  • Something else 🤷‍♂️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment