captivationsoftware / react-sticky

<Sticky /> component for awesome React apps
MIT License
2.64k stars 386 forks source link

Add throttleRendering prop useful for expensive render calls #257

Closed omulet closed 5 years ago

omulet commented 6 years ago

Sometimes, the function supplied to <Sticky> element might be expensive. This prop, allows you to skip re-renders when they are not necessary. Added documentation as well.

Thanks

vcarl commented 6 years ago

Hey @omulet thanks for contributing!! Under what circumstances can you not rely on shouldComponentUpdate?

omulet commented 6 years ago

Hey @vcarl ,

We can always use shouldComponentUpdate but what I am suggesting here, is that shallow comparison of props and state may be inefficient in some circumstances and that will harm performance. I am using this library in a couple of projects and never use distanceFromTop or distanceFromBottom. That's why I think the throttleRendering might be useful for others as well.

Thanks,

vcarl commented 6 years ago

Ah, sure that makes a lot of sense. I haven't used those values in my code either. I don't like boolean flags, but any other optimization would be a breaking change. I think I'm down to merge this, but I want to poke it a bit more before I do. Can go out as a minor release.

vcarl commented 6 years ago

This does a ton to reduce redundant renders, but it also looks like it doesn't properly trigger renders when scrolling past the edge. https://gfycat.com/BeneficialFinishedHuia

omulet commented 6 years ago

Good catch @vcarl . I'll take a look these days and come back with a fix.

Thanks

vcarl commented 5 years ago

@omulet did you have a chance to look into this further? I'm going to close it in a few days if we can't make forward progress on it.