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

Improving offset and adding resize listener #69

Merged
merged 15 commits into from
Apr 5, 2017

Conversation

falcon1kr
Copy link
Collaborator

improved offset props to apply with other use cases, including partialVisibility
Also added optional window resize event listener enabled with resizeCheck props which can be used with resizeDelay

Christian Davis and others added 6 commits January 17, 2017 15:47
Throttle is similar to a debounce but it limits a function to be called
a max number of times per time limit, instead of just delaying the
call. Also moved the findDOMNode call to didMount, since not necessary
to call every time Check is called
@roopemerikukka
Copy link
Collaborator

👍

@eek
Copy link
Collaborator

eek commented Mar 15, 2017

Awesome! I was looking for improvements, and the function from #63 would be great to actually replace my original debounce implementation.

The problem I have is that on mobile the scroll slows down and until it stops, the debounce is not triggered, and that makes things to seem to take forever.

Later-edit: Actually, nevermind, debouce is great on desktop and the throttle from #63 on mobile ^.^

@eek
Copy link
Collaborator

eek commented Mar 24, 2017

@falcon1kr, I have forked you to use your version on our production build, and I have instantly noticed that while you have my debounce fix regarding findDomNode on unMounted components, the debounce function that you moved it in addEventListener is missing it's most important line:

this.lastTimeout = timeout;

after

timeout = setTimeout(later, delay || 0);

I see that clearTimeout(this.lastTimeout); is still there but lastTimeout no longer existed.

context.check.apply(context, args);
};
clearTimeout(timeout);
timeout = setTimeout(later, delay || 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this.lastTimeout = timeout; back under timeout = setTimeout(later, delay || 0);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eek, clearTimeout(this.lastTimeout); was left behind as a mistake. From my understanding, this.lastTimeout was added to track the timeout object so we can cleanup on unmount. But, with the addition of the resize listener, I needed to keep track of multiple timeouts separately to clean up. So I moved it into the "debounce info" object that gets created when addEventListenerWithDebounce is called (timeout property). However, looking back, I realized I made a mistake of statically referring to the timeout object when creating the debounce info object ... I'll fix that to replace it with a function so we can use closure to access the "last" timeout object that's created

@falcon1kr falcon1kr force-pushed the master branch 4 times, most recently from e675ffe to b3976f4 Compare March 24, 2017 19:13
@falcon1kr
Copy link
Collaborator Author

falcon1kr commented Mar 24, 2017

  1. Just addressed the issue mentioned by @eek
  2. merged in the throttle changes made by @ChristianDavis with small changes. instead of throttleScroll and throttleLimit props, my code looks for scrollThrottle > -1 along with scrollCheck. scrollThrottlesupersedesscrollDelay`.
  3. resizeThrottle is added in to support throttling for resize event.
  4. does proper cleanup of the timeout for both delayed and throttled case
  5. updated the README to reflect throttle

@joshwnj
Copy link
Owner

joshwnj commented Apr 3, 2017

Thanks @falcon1kr this is looking good :) Before we merge, is the change to offset the only breaking change? Or are there other things existing users will need to do when upgrading?

@joshwnj
Copy link
Owner

joshwnj commented Apr 4, 2017

Here's how I'm thinking about addressing backwards-compatibility: re-add some of the removed code deleted in this PR, but with a deprecation warning. We can even inform the user exactly how they should upgrade:

+    // Deprecated options for calculating offset.
+    if (typeof offset.direction === 'string' &&
+        typeof offset.value === 'number') {
+      console.warn('[notice] offset.direction and offset.value have been deprecated. They still work for now, but will be removed in next major version. Please upgrade to the new syntax: { %s: %d }', offset.direction, offset.value)
+
+      isVisible = isVisibleWithOffset(offset, rect, containmentRect);
+    }

If nobody has objections I'll merge this PR in the next few days, along with the above modification (and some updates to proptypes and tests).

@joshwnj joshwnj merged commit 34ffcf6 into joshwnj:master Apr 5, 2017
@joshwnj
Copy link
Owner

joshwnj commented Apr 5, 2017

published to v3.8.0

thanks everyone for your help, and your patience getting this big change through :)

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.

4 participants