Avoid request on same page - #152
Conversation
Closes hotwired#42 Navigate with a skip link within our Functional test suite, assert that the correct element is scrolled to, the page's Location path and hash are correct, and that the initial tab stop occurs after the skipped-to content.
getAnchor returns an empty string for blank hashes (#) and undefined when no hash is set at all
Prevent reloading pages when tapping same-page anchors
…to_same_page_anchor Fix navigating back to same page anchor
seanpdoyle
added
bug
db99401 to
e92d9f8
Compare
|
Hey @ozguruysal, I see that we didn't get this into main yet. If you're still interested pushing this forward, could you resolve the conflicts? I'll get it into main. |
|
hi @dhh I guess I managed to resolve the conflicts. I originally sent the PR to |
|
Not sure what's going on here but if I fix the TS error that triggers the build error it causes a conflict. If I don't fix it, no conflict but build fails. @dhh can you help fix this? |
|
Looks like there's still a merge conflict? |
|
Correct but fixing it causes the build to fail. See here. |
|
Ah, I see. That error requires us to make sure that the argument is not null. Either through the TS bang method or a guard clause. |
|
Yes, I fixed it here, but now it complains about the conflict. 😕 |
|
@ozguruysal I think the problem is that you need to rebase this branch, the commits from the print below aren't related to your changes, right? |
|
Thanks @marcelolx. @dhh I guess I've finally managed to fix the problems. Edit: If this is a dirty solution, we can close this PR and I can send a clean one based off of the main branch. |
|
This includes a different way to fix the history pop issue that I fixed in another branch. So let's at least remove that. Don't know if @domchristie have any other observations on this that we should incorporate as well? |
|
There's a fair bit of history on this, and I've not gone through these changes fully, but on first look I think it'd be good to reinstate the navigation tests. Also, I'm not totally sure how new native adapters interact with Turbo, but I remember having to update the turbolinks-ios adapter to be compatible with this change. I can have a closer look tomorrow. |
|
Just let me know if you'd prefer a new PR as this is getting way too complicated. The fix I I introduced is a simple one anyways. |
|
@ozguruysal Maybe it is easier to split this PR into a few smaller chunks. |
|
I think main...domchristie:skip-link-2 is clean and pretty close. Just checking if there's anything else needed to add… |
|
Would be great to get that fix in before we consider going with Turbo 7.0.0 final 👍 |
|
@domchristie feel free to integrate this fix to your branch. Previously I sent the PR to your |
|
@ozguruysal could you explain why that order matters? as opposed to: https://github.com/hotwired/turbo/pull/298/files#diff-78d8451f964182fd51330bac500ba6e71234f81aad9e8a57e682e723d0f517b3R368 |
|
I explained the issue here. Overall, I guess if the target link is an anchor on the same page, there is no point in making any request so it's logical to check that first and return false quickly. |
|
@ozguruysal Thanks. I've added your fix here: 0e4193f. |
|
Let's continue the work in #298. |
5 participants
Aims to fix the issue explained at #42 (comment).