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

Add useAnimateScroll hook #435

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

LauraBeatris
Copy link

@LauraBeatris LauraBeatris commented Jun 11, 2020

As discussed i issue #430, the animateScroll module isn't working when it's called in a useEffect hook on the component mount.

It's a better approach to create a hook to fix that problem instead of relying on the need of creating a ref to verify if the tree has loaded, and then execute the effect with the animateScroll method.

The hook will return a ref that will be used as the scroll container, and behind the scenes it will take care of calling it inside of a useEffect call

useEffect(() => {
    if (containerRef && containerRef.current) {
      method({
        container: containerRef.current,
        ...options,
      });
    }
  }, [
    containerRef,
    method,
    options,
  ]);

We must discuss some points here, this library uses the version ^16.0.0 of React, and hooks only are supported from version 16.8 onwards.

I upgraded React to 16.13.1, but we have to think if it's going to lead to some kind of breaking changes in other projects.

Also, I added a babel plugin to support spread operator.

@fisshy
Copy link
Owner

fisshy commented Jun 12, 2020

Great! Will look into it, otherwise we will release it as 1.8 with breaking changes, it's time to upgrade it anyway.

@alexrabin
Copy link

@fisshy why hasn't this been merged yet? Been over a year and a half

@fisshy
Copy link
Owner

fisshy commented Jan 27, 2023

@fisshy why hasn't this been merged yet? Been over a year and a half

I think I began working on a hook-first approach but ended up not doing it, and now its been forgotten until you woke it up.

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