refactor scroll navigation/restore #750

Merged

neb-b merged 2 commits from scroll-issues into master

2017-11-17 22:35:04 +01:00

neb-b commented

2017-11-15 18:48:21 +01:00

(Migrated from github.com)

Doing some refactoring to how the scroll restore works. This fixes the issue where after navigating, pages were scrolled down a bit.

This gets ride of the need to use a setTimeout which was required to let the new page load before scrolling. We can handle that inside the app component which I think makes more sense because it is interacting with the DOM. I also added throttling to the scroll listener, no need to constantly fire events when scrolling.

After (not sure why my mouse disappeared)

Some things to think about/future ideas

Do we want to restore for all pages? Forwards and back? Not really sure.
If users click the settings icon while on the settings page, maybe scroll to the top instead of doing nothing?

Doing some refactoring to how the scroll restore works. This fixes the issue where after navigating, pages were scrolled down a bit. This gets ride of the need to use a `setTimeout` which was required to let the new page load before scrolling. We can handle that inside the app component which I think makes more sense because it is interacting with the DOM. I also added throttling to the scroll listener, no need to constantly fire events when scrolling. #### After (not sure why my mouse disappeared) ![scroll](https://user-images.githubusercontent.com/16882830/32857350-876a9276-ca15-11e7-8cb5-4390b50f6cc7.gif) #### Some things to think about/future ideas Do we want to restore for all pages? Forwards _and_ back? Not really sure. If users click the `settings` icon while on the settings page, maybe scroll to the top instead of doing nothing?

liamcardenas (Migrated from github.com) reviewed 2017-11-15 18:48:21 +01:00

kauffj (Migrated from github.com) requested changes 2017-11-15 22:43:04 +01:00

kauffj (Migrated from github.com) left a comment

In addition to the issue highlighted below, this code did not work for me. When navigating to a new page after scrolling, the app still retained a scrolled position rather than scrolling to the top.

ui/js/component/app/view.jsx

kauffj (Migrated from github.com) commented

2017-11-15 22:37:54 +01:00

This is easy, but it is not simple.

(This is a distinction you will see me make often. Please watch "Simple Made Easy" by Rich Hickey if you have never seen it. It's relatively short and funny and covers one of the most important lessons in programming. You too @liamcardenas)

If all we need is a throttle method, rather than bringing in an entire library, I'd propose simply bringing that method or an analogous one in directly and making it a core part of our code.

From a quick Google, there's no shortage of implementations for throttling javascript events. Here are several in one thread: https://stackoverflow.com/questions/27078285/simple-throttle-in-js

This is easy, but it is not simple. (This is a distinction you will see me make often. Please watch "Simple Made Easy" by Rich Hickey if you have never seen it. It's relatively short and funny and covers one of the most important lessons in programming. You too @liamcardenas) If all we need is a throttle method, rather than bringing in an entire library, I'd propose simply bringing that method or an analogous one in directly and making it a core part of our code. From a quick Google, there's no shortage of implementations for throttling javascript events. Here are several in one thread: https://stackoverflow.com/questions/27078285/simple-throttle-in-js

neb-b commented

2017-11-16 19:01:28 +01:00

(Migrated from github.com)

Added the redux to this changelog because I didn't add it to the other PR

kauffj (Migrated from github.com) requested changes 2017-11-17 15:59:43 +01:00

kauffj (Migrated from github.com) left a comment

One small proposed name change, otherwise looks great. I also tested again and confirmed it is working for me.

ui/js/redux/selectors/navigation.js

kauffj (Migrated from github.com) commented

2017-11-17 15:55:47 +01:00

This function returns a single entry in the history stack. I would propose a name that better reflects this, perhaps something like selectCurrentHistoryEntry (or use "peek" somehow if we want to keep stack terminology consistent).

This function returns a single entry in the history stack. I would propose a name that better reflects this, perhaps something like `selectCurrentHistoryEntry` (or use "peek" somehow if we want to keep stack terminology consistent).