Allow setting of scroll position based on browser history state. #5006
Conversation
rtibbles
force-pushed
the
scroll_position
branch
from
964ee39 to
4dc6b1f
Compare
Codecov Report
|
rtibbles
force-pushed
the
scroll_position
branch
from
f908b4e to
64702fe
Compare
CoreBase is not reinitialized on navigation.
rtibbles
force-pushed
the
scroll_position
branch
from
64702fe to
f9ee902
Compare
rtibbles
dismissed
ralphiee22’s stale review
I updated behaviour after this review was submitted, so probably needs re-review!
|
Is the scroll position supposed to be unique for each page? The current behavior seems to apply the same position to the previous page even if the position was set at an even earlier page. For example
|
|
Hrm, good point - need to clear the vuex data when leaving a page, so that it is only set from the history state. |
|
Or rather, not the vuex state, but the window offset value - if you navigate to pages without ever scrolling, that doesn't get reset. |
Only conditionalize scroll setting on loading state.
| // Create a watcher to monitor changes in loading | ||
| // to try to set the scrollHeight after the contents | ||
| // have loaded. | ||
| this.unwatchScrollHeight = this.$watch('loading', () => { |
There was a problem hiding this comment.
I wonder if unwatchScrollHeight should be defined on data and initialized to undefined for explicitness? Seems a little weird to just slap it onto the view model.
There was a problem hiding this comment.
Why? It doesn't need to be reactive, putting it on the view itself is exactly the right place for it, the only thing that might be better is prefixing it with an underscore, but I really don't see any benefit to having it be a watched bit of internal state.
There was a problem hiding this comment.
Yeah agreed – I don't like the fact that it's reactive either. The thing that bothered me is to add it to the vm namespace without declaring it anywhere.
There's a lively discussion here so apparently this is something that keeps people up at night :)
Looks like the original strategy you used is likely fine, and another common pattern is to put it inside $options
There was a problem hiding this comment.
I do find it a little odd that the github issue is so long. Either you put it in a namespace, or you put it right on the object. Vue doesn't, and shouldn't care!
Summary
Monitors CoreBase scrolling, and uses that information to update window.pageYOffset to update the history state.
Sets a vuex state element to indicate the desired starting scroll position for the core base - this might be better implemented as an event, but this was easier.
CoreBase adds a watcher to this vuex state in the created property to set the scrollTop to this value when the router has set it.
Reviewer guidance
Does it provide the intended scrolling behaviour?
Could this be implemented in a better way? In what way might this be brittle?
References
Fixes #3659
Contributor Checklist
PR process:
Testing:
Reviewer Checklist
yarnandpip)