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.

Sign up for GitHub

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

Jump to bottom

[WIP] Remember scroll position on error #911

Merged
merged 6 commits into from
Jan 31, 2017
Merged

[WIP] Remember scroll position on error #911

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed81a27
Remember scroll position on error
timweprovide Jan 28, 2017
7441832
Added comment + check if lastScroll was set
timweprovide Jan 28, 2017
e77d787
Remove check for lastAppProps
timweprovide Jan 29, 2017
4f8334e
Use events to make scroll persistence dev-only
timweprovide Jan 29, 2017
41525fe
Return EventEmitter from next()
timweprovide Jan 30, 2017
d3ba6a2
Update next-dev.js
arunoda Jan 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const {
const Component = evalScript(component).default
const ErrorComponent = evalScript(errorComponent).default
let lastAppProps
let lastScroll

export const router = createRouter(pathname, query, {
Component,
Expand Down Expand Up @@ -65,10 +66,27 @@ async function doRender ({ Component, props, err }) {
props = await loadGetInitialProps(Component, { err, pathname, query })
}

if (Component === ErrorComponent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happen if we render the Error component twice. Then it might override the actual lastScroll
Could you test for that case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, good catch 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a comment on why we need to keep this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

lastScroll = {
x: window.pageXOffset,
y: window.pageYOffset
}
}

Component = Component || lastAppProps.Component
props = props || lastAppProps.props

const appProps = { Component, props, err, router, headManager }
lastAppProps = appProps
ReactDOM.render(createElement(App, appProps), container)

if (lastScroll &&
Component !== ErrorComponent &&
lastAppProps.Component === ErrorComponent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do this check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It basically makes sure that the Component being rendered is a page and not an ErrorComponent. Also checks if the last rendered component was an error, since we don't want lastScroll preservation on route changes and normal HMR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I got it.
I mean only this check: lastAppProps.Component === ErrorComponent.

Since we also check for lastScroll, seems like this is like a always true clause.
Give it a try to remove it and test. If we need it, keep it like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I removed it. Think the addition of lastScroll to the set check solved what I was trying to check before.

// Restore scroll after ErrorComponent was replaced with a page component by HMR
const {x, y} = lastScroll
window.scroll(x, y)
lastScroll = false
}

lastAppProps = appProps
}