Fix the hash prop on Links #2224
Conversation
| @@ -289,23 +289,27 @@ describe('A <Link>', function () { | |||
| // just here to make sure click handlers don't prevent it from happening | |||
| } | |||
| render() { | |||
| return <Link to="/hello" onClick={(e) => this.handleClick(e)}>Link</Link> | |||
| return <Link to="/hello" hash="#world" onClick={(e) => this.handleClick(e)}>Link</Link> | |||
There was a problem hiding this comment.
Let's add a query prop here as well, to be a bit more stringent.
There was a problem hiding this comment.
And I'll wait for the +1 next time 😄
|
Oh shit, merged too quickly. I was waiting on tests to pass. I'll follow up on the comments. |
|
I think that will still not jump to the fragment, made similar fix in my local branch initially, but it only solved url. |
|
np :) In general, it's probably best if we all wait to get at least one 👍 from another collab before merging. |
|
@aszwemin Jumping to the fragment is a scrolling problem, which is a separate issue. |
|
It's because the URL is being constructed the wrong way. The hash fragment and query string are out of order. I'm going to submit an issue on |
|
Do we need any sanity checks here when using hash history? |
|
Well, we're now testing another aspect of it (I'd love to get some coverage reports generated...) but I think it's warranted. But once the bug in expect(spy).toHaveBeenCalledWith({ you: 'doing?' }, '/hello#world?how=are')changes to: expect(spy).toHaveBeenCalledWith({ you: 'doing?' }, '/hello?how=are#world')So, it's not so much of a sanity check. It's actually testing the valid construction of a URL. |
Fixes #2216