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

Allow scroll-to-top with shallow routing #21606

Closed
stefanprobst opened this issue Jan 27, 2021 · 12 comments · Fixed by #24888
Closed

Allow scroll-to-top with shallow routing #21606

stefanprobst opened this issue Jan 27, 2021 · 12 comments · Fixed by #24888
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@stefanprobst
Copy link
Contributor

What version of Next.js are you using?

10.0.6-canary.9

What version of Node.js are you using?

14.15.4

What browser are you using?

Firefox 84

What operating system are you using?

Ubuntu 20.04

How are you deploying your application?

Vercel

Describe the Bug

Recently (i think 10.0.6-canary.8) the behavior for shallow routing was changed to not scroll-to-top anymore. While this makes sense as a default behavior, it should still be possible to force scrolling by providing <Link shallow scroll> or router.push(path, undefined, { shallow: true, scroll: true ).

Expected Behavior

<Link shallow scroll> and router.push(path, undefined, { shallow: true, scroll: true ) should scroll to top.

To Reproduce

// pages/index.js
import Link from "next/link";

export default function Home() {
  return (
    <main>
      <h1>Scroll down</h1>
      <div style={{ marginTop: "120vh", padding: "60px" }}>
        <Link href={{ query: { filter: "new" } }} shallow scroll>
          <a>Click me</a>
        </Link>
      </div>
    </main>
  );
}
@stefanprobst stefanprobst added the bug Issue was opened via the bug report template. label Jan 27, 2021
@timneutkens
Copy link
Member

Feel free to send a PR.

@timneutkens timneutkens added good first issue Easy to fix issues, good for newcomers kind: story and removed bug Issue was opened via the bug report template. labels Jan 28, 2021
@timneutkens timneutkens added this to the backlog milestone Jan 28, 2021
@Timer Timer removed this from the backlog milestone Jan 28, 2021
@jigsawpieces
Copy link

jigsawpieces commented Jan 31, 2021

This functionality already exists but the official docs has not been updated yet. See #20606 for more information.

You can do:

router.push(path, path, { scroll: false })

to disable the auto scroll.

@stefanprobst
Copy link
Contributor Author

For shallow routing, this is no longer true with v10.0.6. see #21437

@vercel vercel deleted a comment Feb 6, 2021
@Alexn117
Copy link

I experienced this today as well. Although there are cases where with shallow routing one might not want the page to scroll up, I think there are some where it's convenient. For example, using shallow routing to change pages via query string params (for frontend pagination without refetching data).

<Link href="/posts?page=2" shallow>
  <a>Naturally start new page at the top</a>
</Link>

The docs currently say that the default behavior is to obey scroll to top, but this is not happening with shallow set to true at the moment.

The default behavior of Link is to scroll to the top of the page.

@luismramirezr
Copy link

luismramirezr commented Mar 29, 2021

I experienced this today as well. Although there are cases where with shallow routing one might not want the page to scroll up, I think there are some where it's convenient. For example, using shallow routing to change pages via query string params (for frontend pagination without refetching data).

<Link href="/posts?page=2" shallow>
  <a>Naturally start new page at the top</a>
</Link>

The docs currently say that the default behavior is to obey scroll to the top, but this is not happening with shallow set to true at the moment.

The default behavior of Link is to scroll to the top of the page.

I have the same problem. I'm doing pagination based on query parameters and only the initial fetch is made with SSR. When the user changes a page, the query is updated and then the fetch is made on the client-side.

I would like to have the option to scroll to the top after a shallow route push.

@chinanderm
Copy link

I have the same exact use case as @luismramirezr. Bummer that this broke.

@rewandy
Copy link

rewandy commented Mar 31, 2021

Seeing the same "issue" here, although I can understand why it was made to behave this way. We have one case where scrolling to the top is exactly what we want, because the content being swapped is near the top of the page.

On the other hand, we have a different case where the paginated content lives much further down the page. In that scenario, we actually don't want to scroll all the way to the top of the page, because that would blow way past the content being refreshed and would be even worse than staying in the same spot.

IMO, the "safest" thing to do is append an anchor link (ex. #post-results) at the end of the shallow URL:

<Link href="/posts?page=2#post-results" shallow>
  <a>Naturally start new page at the top</a>
</Link>

That way you have full control of exactly where the browser should scroll upon clicking the shallow link. In this case, we may want to scroll up a bit to where the list of posts begins.

It's a bit more work, but it's the only reasonable approach because only you know where that particular content actually starts on the page.

@chrisneven
Copy link
Contributor

Seeing the same "issue" here, although I can understand why it was made to behave this way. We have one case where scrolling to the top is exactly what we want, because the content being swapped is near the top of the page.

On the other hand, we have a different case where the paginated content lives much further down the page. In that scenario, we actually don't want to scroll all the way to the top of the page, because that would blow way past the content being refreshed and would be even worse than staying in the same spot.

IMO, the "safest" thing to do is append an anchor link (ex. #post-results) at the end of the shallow URL:

<Link href="/posts?page=2#post-results" shallow>
  <a>Naturally start new page at the top</a>
</Link>

That way you have full control of exactly where the browser should scroll upon clicking the shallow link. In this case, we may want to scroll up a bit to where the list of posts begins.

It's a bit more work, but it's the only reasonable approach because only you know where that particular content actually starts on the page.

IMO the scroll option should just take precedence over opinionated default behavior when it's explicitly set.

@chinanderm
Copy link

IMO the scroll option should just take precedence over opinionated default behavior when it's explicitly set.

Exactly.

@chrisneven
Copy link
Contributor

Feel free to send a PR.

Hi @timneutkens, we'd really like to see this fixed and as you might've noticed I opened a pull request with regard to this issue.

Alvast bedankt!

@timneutkens
Copy link
Member

Posted a reply here: #24888 (review)

@kodiakhq kodiakhq bot closed this as completed in #24888 Jun 8, 2021
kodiakhq bot pushed a commit that referenced this issue Jun 8, 2021
## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added

fixes [#21606](#21606)

### Description
When using shallow routing and wanting to scroll to top by setting the `scroll` option to `true` it didn't work. This PR fixes this issue.
flybayer pushed a commit to blitz-js/next.js that referenced this issue Jun 16, 2021
…#24888)

## Bug

- [x] Related issues linked using `fixes #number`
- [ ] Integration tests added

fixes [vercel#21606](vercel#21606)

### Description
When using shallow routing and wanting to scroll to top by setting the `scroll` option to `true` it didn't work. This PR fixes this issue.
@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants