Issue #312 home page scroll position on back navigation #331

2017-07-06T05:19:40+02:00

akinwale commented

2017-07-06 05:19:40 +02:00

(Migrated from github.com)

This was a bit difficult to figure out, since componentDidMount is meant to execute after render (based on what I have read about the React component life cycle), but for some reason, calling window.scrollTo directly in this method did not work. I also tried in the componentDidUpdate method. I ended up using a setTimeout to call the scrollTo method for the page to actually scroll to the previous position.

This was a bit difficult to figure out, since `componentDidMount` is meant to execute after `render` (based on what I have read about the React component life cycle), but for some reason, calling `window.scrollTo` directly in this method did not work. I also tried in the `componentDidUpdate` method. I ended up using a `setTimeout` to call the `scrollTo` method for the page to actually scroll to the previous position.

kauffj commented

2017-07-06 22:05:53 +02:00

(Migrated from github.com)

@akinwale Is there any way to use Chrome's native scroll position retention? I suspect not, since we're controlling navigation, but perhaps it's possible.

Additionally, I'm considering whether or not we want this on all pages. I think most users would expect "back" to take them to the position they were previously in (I understand the ticket didn't stipulate this.)

Finally, I'd like to avoid using the session for this in favor of the redux store.

@akinwale Is there any way to use Chrome's native scroll position retention? I suspect not, since we're controlling navigation, but perhaps it's possible. Additionally, I'm considering whether or not we want this on all pages. I think most users would expect "back" to take them to the position they were previously in (I understand the ticket didn't stipulate this.) Finally, I'd like to avoid using the session for this in favor of the redux store.

kauffj commented

2017-07-13 16:31:01 +02:00

(Migrated from github.com)

@akinwale want to come back to this one? I thought about merging as-is, but at a minimum it needs to only happen on "back" events, not all navigations to Discover.

akinwale commented

2017-07-13 16:33:48 +02:00

(Migrated from github.com)

@kauffj I haven't yet figured out how to detect navigation back events in React, but I've made changes to make use of the Redux store. I'll push a new commit after I'm done.

kauffj commented

2017-07-13 16:35:03 +02:00

(Migrated from github.com)

@akinwale look at doHistoryBack in ui/actions/app.js

@akinwale look at `doHistoryBack` in `ui/actions/app.js`

akinwale commented

2017-07-13 17:35:24 +02:00

(Migrated from github.com)

@kauffj Pushed a new commit.

kauffj (Migrated from github.com) reviewed 2017-07-13 21:40:57 +02:00

ui/js/actions/app.js

					
				@ -65,2 +65,4 @@

				    history.back();

				    dispatch({

				      type: types.HISTORY_BACK,

kauffj (Migrated from github.com) commented

2017-07-13 21:40:57 +02:00

Is this supposed to be HISTORY_BACK_COMPLETED? If not, I don't think doHistoryBackCompleted ever happens. If so, I don't think we're guaranteed the render happens while navigatingBack is true.

The other relevant code to know here (if you haven't discovered it already) is the popstate event listener in ui/js/main.js. This is what history.back() is triggering.

Is this supposed to be `HISTORY_BACK_COMPLETED`? If not, I don't think `doHistoryBackCompleted` ever happens. If so, I don't think we're guaranteed the render happens while `navigatingBack` is true. The other relevant code to know here (if you haven't discovered it already) is the popstate event listener in `ui/js/main.js`. This is what history.back() is triggering.

akinwale (Migrated from github.com) reviewed 2017-07-13 23:56:01 +02:00

ui/js/page/discover/view.jsx

					
				@ -40,0 +57,4 @@

				  }

				  handleScroll() {

				    lbry.setClientSetting("prefs_scrolly", window.scrollY);

akinwale (Migrated from github.com) commented

2017-07-13 23:56:01 +02:00

@kauffj This delegates to doHistoryBackCompleted(). I called it at this point because the popstate event was not being fired after clicking the back button. Also, HISTORY_BACK occurs before CHANGE_PATH, and this happens before componentDidMount. I needed a way to reliably detect that the user navigated using the back button and being able to check the flag when the component mounts was how I could do it (although I figure there might be a better way but I'm not seeing it at this point).

@kauffj This delegates to `doHistoryBackCompleted()`. I called it at this point because the `popstate` event was not being fired after clicking the back button. Also, `HISTORY_BACK` occurs before `CHANGE_PATH`, and this happens before `componentDidMount`. I needed a way to reliably detect that the user navigated using the back button and being able to check the flag when the component mounts was how I could do it (although I figure there might be a better way but I'm not seeing it at this point).

6ea86b96 commented

2017-07-19 08:22:02 +02:00

(Migrated from github.com)

I think this can be simplified. We should be able to monitor the scroll position globally from the app component right? All back events are handled by ui/js/main.js. In there we can just pass a scroll variable to the doChangePath action to tell it whether to scroll to a previous position or not.

I think this can be simplified. We should be able to monitor the scroll position globally from the app component right? All back events are handled by `ui/js/main.js`. In there we can just pass a `scroll` variable to the `doChangePath` action to tell it whether to scroll to a previous position or not.

6ea86b96 commented

2017-07-19 09:45:13 +02:00

(Migrated from github.com)

In fact actually we should just be able to store the scroll position in the history state and scroll ourselves, on all pages.

filipnyquist commented

2017-07-19 10:32:16 +02:00

(Migrated from github.com)

@6ea86b96 seems like a easier and better way to do it, any downsides?

6ea86b96 commented

2017-07-19 10:38:28 +02:00

(Migrated from github.com)

@filipnyquist @kauffj I don't see any immediate downsides. I've implemented it here https://github.com/lbryio/lbry-app/pull/365

Now scrollY is retained over the full history.

@filipnyquist @kauffj I don't see any immediate downsides. I've implemented it here https://github.com/lbryio/lbry-app/pull/365 Now scrollY is retained over the full history.

Sign in to join this conversation.

No reviewers