Skip to content
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

Adding possibility to use built-in observer #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ku8ar
Copy link

@ku8ar ku8ar commented Jul 8, 2019

In one of the projects I have hundreds of pictures, which in addition must be tracked using GTM. Currently, each picture has two same observers. This fix will significantly improve performance in my project...

@ku8ar
Copy link
Author

ku8ar commented Jul 24, 2019

@fpapado Is There Anybody Out There?

@fpapado
Copy link
Owner

fpapado commented Jul 24, 2019

Hi @ku8ar, it's me out here :D

I saw this PR initially, left a note to get back to it, and the summer holiday season came around.

Part of what you are requesting has come up before (#26, and the inverse in #20). I would like to know a bit more about your use case. You mention performance would improve, can you tell me a bit more about that? Is it traceable to IntersectionObserver observing too many things? Maybe there's a before/after comparison with this PR?

One of the things that came up in #26, is that with hundreds of images (a gallery, perhaps), one might run into issues anyway (either the DOM, or keeping the components around). In that case, it seems preferable to use some sort of pagination, rather than focusing just on the IntersectionObserver parts. However, this might not be true for your use case! That's why I would like to know more about it.

The changes in this PR seem good in terms of functionality. I'd like to think some more about how we expose it in the API. I think triggerOnce is one of the guarantees we want in the component, but it would probably work ok due to how the states are set up. Exposing onChange might prohibit future changes to the API, if consumers start relying on it too much. I'm keeping it open until we've looked into the use cases, let's see!

@ku8ar
Copy link
Author

ku8ar commented Aug 9, 2019

Hello @fpapado Sorry for the late reply. Now I was on vacation;)

The team I work with, creates a clothes shop. We display infinity card list. Items are added to the list after user scroll down.
Each item that appears on the list fetch photo using this component, and in addition sends GTM information that the photo has been seen.

Currently, unfortunately, we cant easily add react-window, because one of the requirements is that the user can go back to the list page (saving scroll position). So the only way is to optimize the entire list.
Reducing the number of observers helps a little.
In addition, I don't know if you noticed, I speeded up the PR code slightly (throwing arrow binding).

So what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants