Skip to content

Commit

Permalink
Fix broken hash scroll logic (#4766)
Browse files Browse the repository at this point in the history
`<Container>` does not receive any property. There is no way the *scrollToHash* logic can work right now. I believe it's a regression. It was working fine at some point. I'm sorry, I'm too lazy to add a test.

This fix was tested on Material-UI 👌.

This bug reproduction is the following:
As soon as you want to transition to a new page with a hash. The scroll doesn't change.
- start on pageA
- you scrollTop to 100
- you move to pageB#hash
- you stay at scrollTop 100, but #hash is at scrollTop 400.
  • Loading branch information
oliviertassinari authored and timneutkens committed Aug 11, 2018
1 parent 3eda0c0 commit b02fff6
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 4 deletions.
5 changes: 2 additions & 3 deletions lib/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export class Container extends Component {
}

scrollToHash () {
const { hash } = this.props
const { hash } = this.context._containerProps
if (!hash) return

const el = document.getElementById(hash)
Expand All @@ -73,8 +73,7 @@ export class Container extends Component {
}

render () {
const {children} = this.props
return <>{children}</>
return this.props.children
}
}

Expand Down
1 change: 1 addition & 0 deletions test/integration/basic/pages/nav/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export default class extends Component {
Counter: {counter}
</div>
<button id='increase' onClick={() => this.increase()}>Increase</button>
<Link href='/nav/hash-changes#item-400'><a id='scroll-to-hash'>Scroll to hash</a></Link>
</div>
)
}
Expand Down
21 changes: 20 additions & 1 deletion test/integration/basic/test/client-navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ export default (context, render) => {
browser.close()
})

it('should scroll to the specified position', async () => {
it('should scroll to the specified position on the same page', async () => {
let browser
try {
browser = await webdriver(context.appPort, '/nav/hash-changes')
Expand All @@ -272,6 +272,25 @@ export default (context, render) => {
}
}
})

it('should scroll to the specified position to a new page', async () => {
let browser
try {
browser = await webdriver(context.appPort, '/nav')

// Scrolls to item 400 on the page
await browser
.elementByCss('#scroll-to-hash').click()
.waitForElementByCss('#hash-changes-page')

const scrollPosition = await browser.eval('window.pageYOffset')
expect(scrollPosition).toBe(7258)
} finally {
if (browser) {
browser.close()
}
}
})
})

describe('when hash change via A tag', () => {
Expand Down

0 comments on commit b02fff6

Please sign in to comment.