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

Create useWindowScrollLock hook #112

Open
leroykorterink opened this issue Mar 28, 2023 · 4 comments
Open

Create useWindowScrollLock hook #112

leroykorterink opened this issue Mar 28, 2023 · 4 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@leroykorterink
Copy link
Collaborator

leroykorterink commented Mar 28, 2023

We should add the useWindowScrollLock hook.

import { noop } from 'lodash-es';
import { useEffect, useRef } from 'react';

export function useWindowScrollLock(enabled = true): void {
  const scrollTopRef = useRef(0);

  useEffect(() => {
    if (!enabled) {
      return noop;
    }

    // Save previous scroll position so that we can restore it when the lock is disabled
    scrollTopRef.current = window.scrollY;

    const htmlElement = document.firstElementChild as HTMLHtmlElement;
    htmlElement.style.setProperty('overflow-y', 'hidden');

    return () => {
      htmlElement.style.setProperty('overflow-y', 'auto');

      // Restore previous scroll position
      scrollTopRef.current = window.scrollY;
    };
  }, [enabled]);
}
@leroykorterink leroykorterink added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Mar 28, 2023
@ThaNarie
Copy link
Member

A small comment on the enabled hook param.
I saw a similar thing in another hook in the works (useAnimationLoop I believe).

While this allows switching on re-render, it won't give you the option to change it from a useEffect or useCallback. For that, you would need to create a useState outside of this hook to manage it.

However, if we make this hook return a enable/disable function, it might run out of sync with the passed param.

So while I haven't it given enough thought to have a proper solution in mind, it's probably good to think of the best ergonomics of managing this based on example use cases, and document these kind of patterns so our hooks are consistent.

@leroykorterink
Copy link
Collaborator Author

However, if we make this hook return a enable/disable function, it might run out of sync with the passed param.

This is why I prefer to only have the parameter to enable/disable the hook implementation.

const [value, toggle] = useToggle();

useWindowScrollLock(value);
useSomethingElse(value);

useCallback(() => {
  toggle();
}, [toggle])

I think we should make it easy to keep things in sync, using an external state minimises the places that need to be updated when the logic changes. And thus reduces the chance of bugs.

const { toggle } = useWindowScrollLock();
const { toggle: toggleSomethingElse } = useSomethingElse();

useCallback(() => {
  toggle();
  toggleSomethingElse();
}, [toggle, toggleSomethingElse])

@ReneDrie
Copy link
Contributor

Don't you also want to set the current window.scrollY on some other element when locking? Otherwise you would jump to the top

@jesse-mm
Copy link
Collaborator

Keep in mind you probably want to lock x axis as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants