search

streamich / react-use

React Hooks — 👍
http://streamich.github.io/react-use
The Unlicense
41.57k stars 3.13k forks source link

useLockBodyScroll doesn't restore scroll position #840

Open Undistraction opened 4 years ago

Undistraction commented 4 years ago

What is the current behavior?

When useLockBodyScroll is passed a value of false, it does re-enable scrolling, however any previous scroll position is not restored.

What is the expected behavior?

The previous scroll position should be restored to the scrolled element.

A little about versions:

d-asensio commented 4 years ago

Yes, the useEffect should return a cleanup function that restores the scroll. Also, I think that setting the locked parameter to true is a bad practice.

https://github.com/streamich/react-use/blob/16d0ad566bcb3978b069b310e9f7a1b6e8d0815d/src/useLockBodyScroll.ts#L47

Since it will be treated as true when it receives undefined (which is a falsy value) and this really goes against intuitions when it comes to JavaScript. And especially in React in which undefined jsx properties must be treated as false, so imagine the following situation:

<Modal open />
<Modal />

In the first case, the <Modal /> component will receive a open: true prop, but in the second case it will receive open: undefined. And considering the following implementation (which describes a very reasonable use case in my opinion):

function Modal ({ open }) {
  useLockBodyScroll(open)
  ...
}

You will get the behavior that, I guess, is the opposite as the most JavaScript developers would expect at first sight.

I can submit a PR to fix this, but this will imply a BRAKING CHANGE, how should we proceed @streamich ?

The issue was originally found by a member of my team: @oriohol

streamich commented 4 years ago

@d-asensio yeah, that would be great, let's merge it into v14.0 branch

manishsundriyal commented 4 years ago

@d-asensio @streamich We also require a cleanup method for https://github.com/streamich/react-use/issues/858 and I have created a PR for the cleanup method https://github.com/streamich/react-use/pull/1130 However, it does not restore the scroll position. I can add the restore scroll position in the cleanup method that I have already implemented. Any suggestions?

rodrigo-picanco commented 4 years ago

Guys, any news here? Stumbled upon the matter while working and was wandering if u need any help.

rodrigo-picanco commented 4 years ago

Guys, any news here? Stumbled upon the matter while working and was wandering if u need any help.

Just updated to 5.3.2 and it just works! Thank you for the amazing lib <3

manishsundriyal commented 4 years ago

@streamich @Undistraction I think, this has been fixed in https://github.com/streamich/react-use/pull/1130 Can you please recheck and close this?

n4bb12 commented 3 years ago

I also think this is fixed. Using the latest version of react-use, the scroll position gets restored: https://codesandbox.io/s/sweet-nobel-33bhl