vercel / next.js

The React Framework
https://nextjs.org
MIT License
126.91k stars 26.97k forks source link

[Examples] Modal dynamic route doesn't keep scroll position #17400

Closed kyleawayan closed 4 years ago

kyleawayan commented 4 years ago

Bug report

The example from https://github.com/vercel/next.js/tree/canary/examples/with-route-as-modal doesn't keep scroll position when modal is opened using the dynamic routing method.

Describe the bug

I'm not sure if the dynamic routing method is supposed to work like the QueryString routing where it keeps the scrolling position of the main page. I'm not able to use the QueryString method due to getStaticProps() not working as a component correctly (can't use mongodb node driver, it errors saying dns could not be found)

The scrolling position is saved when using the QueryString method after you close the modal. But for the dynamic routing method, it unloads the main page and only shows the modal (as seen in the gif below). When the modal is exited, the main page is reloaded again and the scroll position from the previous context is lost.

To Reproduce

  1. yarn create next-app --example with-route-as-modal with-route-as-modal-app
  2. cd with-route-as-modal-app
  3. yarn dev
  4. Go to localhost:3000 in Safari
  5. Click on one of the numbers under "With Dynamic Routing, and reloads will keep the modal"
  6. Exit out of the modal by clicking on the outer area of the screen.

Expected behavior

When exiting out of the modal, it should keep the scroll position of the main page.

Screenshots

2020-09-27 17 09 00

System information

kyleawayan commented 4 years ago

Tried it on my Windows 10 PC on Firefox. Both of the QueryString routing and dynamic routing don't keep the scroll position. Although the QueryString routing does kinda keep the scrolling position when the window is really wide (like 1729 x 381), it kinda scrolls up a bit from the original scrolling context. Having it in a regular window size doesn't keep the scrolling position.

ludofischer commented 4 years ago

@kyleawayan I’ve found reactjs/react-modal#117 which seems to indicate the scroll to top is a react-modal issue, so not specific to Next. On my machine (Firefox/Linux) I can see the scroll happening as soon as the modal opens.

kyleawayan commented 4 years ago

@ludovicofischer Is this happening with both the QueryString and dynamic routing method? I also see that the main page scrolls to the top when the modal is opened in the QueryString method. But with the dynamic routing method, it unloads the whole main page.

ludofischer commented 4 years ago

@kyleawayan I see this happening just as you describe. This and the surrounding lines might be related to the issue https://github.com/vercel/next.js/blob/816798569a56c97108ecff37a85e6a3fd85648ab/packages/next/next-server/lib/router/router.ts#L428 I was quite surprised as I did not know about the scroll restoration API. I don’t know if there is hope to restore scroll position in the QueryString case, as it could be hard for Next to distinguish a popu-triggered scroll from a user-initiated one. In the dynamic routing case, I am not sure about what’s going on.

kyleawayan commented 4 years ago

@ludovicofischer Yeah I'm not sure if this worked in earlier versions of Next or the modal or something like that. I wish this would work because I have an infinite scrolling gallery with dynamic loading. It's kind of a nightmare to deal with especially with keeping scroll positions... so that's why I wanted to have a modal, especially with the dynamic route so I can keep my current page structure. I'll take a look into this when I have time to see if I can find a solution (maybe, I'm not sure how complicated this may be and I'm not that experienced😭).

HaNdTriX commented 4 years ago

You can disable scroll restoration using the scroll prop on next/link

File: components/Grid.js

import Link from 'next/link'
import styles from './styles.module.css'

export const data = [1, 2, 3, 4, 5, 6, 7, 8, 9]

export default function PostCardGrid() {
  return (
    <div className={styles.postCardGridWrapper}>
      <h2>With QueryString Routing, and a reload won't use the modal</h2>
      <div className={styles.postCardGrid}>
        {data.map((id, index) => (
          <Link key={index} href={`/?postId=${id}`} as={`/post/${id}`}>
            <a className={styles.postCard}>{id}</a>
          </Link>
        ))}
      </div>

      <h2>With Dynamic Routing, and reloads will keep the modal</h2>
      <div className={styles.postCardGrid}>
        {data.map((id, index) => (
          <Link
            key={index}
            href="/article/[articleId]"
            as={`/article/${id}`}
+           scroll={false}
          >
            <a className={styles.postCard}>{id}</a>
          </Link>
        ))}
      </div>
    </div>
  )
}
kyleawayan commented 4 years ago

@HaNdTriX Unfortunately it didn't work for the dynamic routing as it still unloaded the page and didn't restore the scroll position... but it did work with the QueryString routing.

modal

jmfury commented 4 years ago

@kyleawayan I threw up a repo where I've used a couple concepts that break with the original example; namely this is using layouts

Seems to be keeping the scroll as desired via scroll={false} on <Link /> when the page components via layouts are essentially all the same.

This was my hunch on how to achieve what you're looking for and I tried to be descriptive in the Headings to illustrate the differences in functionality for query string vs. dynamic routing.

Give it a look and let me know of any questions 👍 https://github.com/jmfury/nextjs-route-as-modal-plus-scroll

kyleawayan commented 4 years ago

@jmfury Wow thank you so much for this! I cloned your repo and it works on my end! Now I just need to figure out how to incorporate it to my website. Although reloading will lose the scroll position, it's good enough as people looking through my gallery won't really need to reload. But you got the exact thing I was looking for!

jmfury commented 4 years ago

@jmfury Wow thank you so much for this! I cloned your repo and it works on my end! Now I just need to figure out how to incorporate it to my website. Although reloading will lose the scroll position, it's good enough as people looking through my gallery won't really need to reload. But you got the exact thing I was looking for!

@kyleawayan Glad to help! I think the key here is eliminating the need for next (react on the client?) to unmount your page component which was the case in the original example. The pages were very different components in dynamic routing navigation.

Reloads are essentially a new server-render so I don't think keeping the scroll is typically desired anyway. It is potentially doable with next/router events... not positive, but I found this along the way as food for thought 🔗

kyleawayan commented 4 years ago

@jmfury Alright so before I begin to try to incorporate this into my website, I looked at the code more closely to understand how everything works. For my website, I need to be able to use getStaticProps() separately on each page, but in your example it looks like the dynamic routing goes to a component called Article.js. I can't use the mongodb driver in a component because the component is only front-end or something like that. I made a drawing on how I understand how its working:

image

I see that there's a page called [articleId].js, which has getStaticProps and getStaticPaths, but it looks like this is only for when a user refreshes the page.

Is this the correct flow of the example? Or is using getStaticProps() from Article.js possible? Or would I need to pass props to the component?

kyleawayan commented 4 years ago

@jmfury Alright so before I begin to try to incorporate this into my website, I looked at the code more closely to understand how everything works. For my website, I need to be able to use getStaticProps() separately on each page, but in your example it looks like the dynamic routing goes to a component called Article.js. I can't use the mongodb driver in a component because the component is only front-end or something like that. I made a drawing on how I understand how its working:

image

I see that there's a page called [articleId].js, which has getStaticProps and getStaticPaths, but it looks like this is only for when a user refreshes the page.

Is this the correct flow of the example? Or is using getStaticProps() from Article.js possible? Or would I need to pass props to the component?

@jmfury Nevermind, I understood the flow wrong. I tried changing the getStaticProps() in [articleId].js to

export function getStaticProps({ params: { articleId } }) {
  return { props: { articleId: "sdfdskfhsdkljh" } };
}

and it reflected on all the pages!

jmfury commented 4 years ago

@kyleawayan Great to hear it's working! Feel free to tag me on any code you can share outside this thread as needed. Happy to assist if any of the concepts are giving you any trouble in your implementation!

kyleawayan commented 4 years ago

@jmfury Thank you! I have one problem, I'm going to try to explain what I have right now... but first let me explain how my website works.

If you go to my website, there is an initial gallery that the user sees. They then can click any photo, and then it will go to like a "sub-gallery" you could say.

My home page gallery (like your Grid component in your example) uses getStaticProps() to get photos for the home page. I then pass that on to my HomeGallery layout which it is then rendered.

But when I go to a "sub-gallery," the layout can't load the initial getStaticProps() which was passed on from the home page gallery since it's not on that page (index.js) anymore.

I'm not sure if your example reloads the whole page, or it's because I'm using getStaticProps(), everything is getting messed up. FYI, I'm using router.push as I can't really map <Link>s to each photo in react-photo-gallery. I even tried making a dummy link with scroll={false} just to try it but it's still resulting in the same issue.

Do you recommend any other ways to circumvent this?

kyleawayan commented 3 years ago

Hi everyone! Just wanted to leave an update here: I got it working with my website! Ends up I really couldn't get the dynamic routing working since my page was re-rendering every time the modal closed... But I found a solution! Instead of using getStaticProps(), I used SWR to fetch data on the client-side. I made an API route that gets the same data that getStaticProps() does, then implemented SWR into a component, and used the QueryString method given in @jmfury's example. So now I basically have a modal component for my "sub-gallery" for the QueryString modal, and a dynamically routed page with basically the same content as the modal component, the only difference is that it's using getStaticProps().

It works perfectly! You can check it out on my website.

Thank you to @jmfury for your example repo and everyone that helped!

kylemh commented 3 years ago

@jmfury @kyleawayan

I've got a similar UI with a list view that opens up a detail view. The only difference is that there exists a Flickr-esque carousel across the detail view (here's a Flickr search... simply click a photo and navigate with and/or .

I feel like my only recourse is to track window.scrollY somewhere and scroll in an effect.

Do y'all have alternate ideas?

kyleawayan commented 3 years ago

@kylemh I have stopped using the method above, since client fetching data just to save a scroll position probably isn't worth the performance/speed trade off. However my new website works similarly, but instead of going to a modal it just goes to another page. This allows for server side rendering for faster performance. I'm using this gist to keep scrolling position.

I'm not sure if infinite scrolling will work with this method though...

jmfury commented 3 years ago

Do y'all have alternate ideas?

I'm happy to take a look at some code if you can link it up.

You can also try to extract the approach I used here in this repo: https://github.com/jmfury/nextjs-route-as-modal-plus-scroll

Hopefully it works for your use case. 🤞

I think if the component tree is organized a certain way it should just work i.e. use a layout component.

The other enabling factor is <Link scroll={false} />:
https://github.com/jmfury/nextjs-route-as-modal-plus-scroll/blob/main/components/Grid.js#L36

Let me know!

kylemh commented 3 years ago

Right, we're following persistent layouts and the 2 pages in question use the same layout; however, unlike modals as routes, we're rendering 2 entirely different pages. The only code each page file shares is the layout.

I guess I could embed routing logic within the layout and match up with your example, but I think I may use the gist Kyle provided instead. Seems a little more obvious/declarative.

jmfury commented 3 years ago

Right, we're following persistent layouts and the 2 pages in question use the same layout; however, unlike modals as routes, we're rendering 2 entirely different pages. The only code each page file shares is the layout.

I guess I could embed routing logic within the layout and match up with your example, but I think I may use the gist Kyle provided instead. Seems a little more obvious/declarative.

Whatever works!

If you're using a layout, a state value could probably be used rather than browser storage (as is the case in that gist), but it sounds like you're on the track you want to be on!

I still prefer to just set up the routes and layout and use <Link scroll={false} />:
https://github.com/jmfury/nextjs-route-as-modal-plus-scroll/blob/main/components/Grid.js#L36

Cheers!

kylemh commented 3 years ago

What a fun day! Next.js actually should've properly sorted me out with using the router methods / Link with scroll=false; however, the real problem was that the scrolling container was actually a virtualized list attached to an element that wasn't window... I adapted the gist's hook to work with a custom element and we're flying now.

balazsorban44 commented 2 years ago

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.