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.

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

[WIP] Remember scroll position on error #911

Merged
merged 6 commits into from
Jan 31, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
16 changes: 15 additions & 1 deletion client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import evalScript from '../lib/eval-script'
import { loadGetInitialProps } from '../lib/utils'

const {
CustomEvent,
__NEXT_DATA__: {
component,
errorComponent,
Expand Down Expand Up @@ -65,10 +66,23 @@ async function doRender ({ Component, props, err }) {
props = await loadGetInitialProps(Component, { err, pathname, query })
}

// Try/catch is needed because IE11 has a CustomEvent implementation without contructor
try {
const event = new CustomEvent('before-reactdom-render', { detail: { Component } })
document.dispatchEvent(event)
} catch (e) {}

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

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

// Try/catch is needed because IE11 has a CustomEvent implementation without contructor
try {
const event = new CustomEvent('after-reactdom-render', { detail: { Component } })
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks not nice to me.
Shall we accept a new callback from the default function of this module and next-dev simply use it.

That's much better and clean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we emit two events. Simply return an EventEmitter from the next() call. Then next-dev can listen it.

I think I like that over the above my suggested approach too.

document.dispatchEvent(event)
} catch (e) {}

lastAppProps = appProps
}
26 changes: 26 additions & 0 deletions client/next-dev.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import patch from './patch-react'
import evalScript from '../lib/eval-script'

const { __NEXT_DATA__: { errorComponent } } = window
const ErrorComponent = evalScript(errorComponent).default

// apply patch first
patch((err) => {
Expand All @@ -20,3 +24,25 @@ function onError (err) {
// so that the current component doesn't lose props
next.render({ err })
}

let lastScroll

document.addEventListener('before-reactdom-render', ({ detail: { Component } }) => {
// Remember scroll when ErrorComponent is being rendered to later restore it
if (!lastScroll && Component === ErrorComponent) {
const { pageXOffset, pageYOffset } = window
lastScroll = {
x: pageXOffset,
y: pageYOffset
}
}
})

document.addEventListener('after-reactdom-render', ({ detail: { Component } }) => {
if (lastScroll && Component !== ErrorComponent) {
// Restore scroll after ErrorComponent was replaced with a page component by HMR
const { x, y } = lastScroll
window.scroll(x, y)
lastScroll = false
}
})