Implement <ScrollRestoration />

[gitstart-app]

Description

Implement <ScrollRestoration />

Refs

https://github.com/twentyhq/twenty/issues/4357

Demo

scroll.restoration.mp4

Fixes #4357

[gitstart-twenty]

Hey @FelixMalfait

In order to use ScrollRestoration we'd need to switch to createBrowserRouter. This means components outside the routes do not have access to react-router-dom hooks. These components include the various providers in App.tsx, some of which use useNavigate or useLocation. I've created custom versions of these hooks that use history to be used outside the router, please check if these initial changes make sense

[FelixMalfait]

Hey @gitstart-twenty ; switching to createBrowserRouter is the right path.

But are you sure the hooks don't work any more?
https://reactrouter.com/en/main/upgrading/v6-data

It seems that they should work (they introduce other hooks like useNavigation, useMatches but don't deprecate others)

You should not directly access history imo, this feels like a bad direction

I would try to stay as close to the upgrade-guide as possible

[gitstart-twenty]

Hey @FelixMalfait,

I think there has been a misunderstanding, allow me clarify on some things.

The hooks still function as expected for components nested within the routes. However, the providers located outside the routes lack access to the react-router-dom context. Previously, we addressed this by wrapping them in BrowserRouter. However, with createBrowserRouter, we have to make a switch to RouterProvider, which does not accept children. Consequently, the previous solution is no longer viable.

For providers situated outside the routes, we'll need to find an alternative method for accessing router-related functionality, as the hooks from react-router won't be available(which is what this PR does).

To sum up, both the old and new hooks(introduced in v6.4) still function inside the routes as usual. The changes outlined in this PR address the necessary modifications to ensure that the providers continue to function correctly, even without the react-router context.

[gitstart-twenty]

Needs react-router context, put under pathless route

[gitstart-twenty]

Need react-router context, put under pathless route with Outlet

[lucasbordeau]

@gitstart-twenty Seems ok for me, do you anything else to move forward ?

[gitstart-twenty]

@gitstart-twenty Seems ok for me, do you anything else to move forward ?

Just needed that green light 🚦

[gitstart-twenty]

Hey @lucasbordeau,

Using createBrowserRouter won't suffice because it relies on the scroll position of the page, whereas our focus is on the scroll position of the table(s). Would it be more practical if we stored the table scroll positions, perhaps in state (maybe using recoil), and retrieved them from there upon returning to the page(s)?

cc @FelixMalfait

[FelixMalfait]

@gitstart-twenty It's nice that you did the migration to data BrowserRouter since we'll have to do it at some point any way so let's get it to the end.

For scrollRestoration on tables (which is what matters most) we'll have to do something custom indeed :(
See remix-run/react-router#10468 (comment)

It will be nicer once we have this implemented #4914

cc @lucasbordeau

[gitstart-twenty]

@gitstart-twenty It's nice that you did the migration to data BrowserRouter since we'll have to do it at some point any way so let's get it to the end.

For scrollRestoration on tables (which is what matters most) we'll have to do something custom indeed :( See remix-run/react-router#10468 (comment)

It will be nicer once we have this implemented #4914

cc @lucasbordeau

@FelixMalfait This is interesting! Looking into it

[FelixMalfait]

@gitstart-twenty not much interesting in the code itself because it's a basic poc we need to adapt it to our codebase (@lucasbordeau would know best) the most interesting part is that they also came to the conclusion that it wasn't possible... Thanks!

[gitstart-twenty]

@gitstart-twenty not much interesting in the code itself because it's a basic poc we need to adapt it to our codebase (@lucasbordeau would know best) the most interesting part is that they also came to the conclusion that it wasn't possible... Thanks!

Of course! Thanks for the update

[lucasbordeau]

Seems like a custom implementation with session storage is the way to go !

[lucasbordeau]

@gitstart-twenty We talked about the implementation, here's the guidelines :

• We don't want to restore scrolling if someone refreshes the page, so maybe session storage is overkill and recoil is enough.
• We have to know if we can actually restore the scroll, due to lazy loading. If the scroll is located on edges page 10, at node 290, and we change page but the table resets its pagination state and we only have 30 nodes, we don't want to scroll to node 290, nor load more 10 times to have the 290th node.

[gitstart-twenty]

Hey @lucasbordeau

We have to know if we can actually restore the scroll, due to lazy loading. If the scroll is located on edges page 10, at node 290, and we change page but the table resets its pagination state and we only have 30 nodes, we don't want to scroll to node 290, nor load more 10 times to have the 290th node.
We're having a challenge, the scroll needs to be triggered after the rows are visible (scrollable). What do you think is the best approach here?

How do we plan on achieving this? Should we limit the amount of scroll possible (maybe not bigger than the screen height)?

[lucasbordeau]

We shouldn't scroll if scroll item index > number of items in the table.

[gitstart-twenty]

We shouldn't scroll if scroll item index > number of items in the table.

Alright!

[lucasbordeau]

Works fine and covers the use case we have we show page / table navigation !

[lucasbordeau]

We shouldn't deactivate this rule, this is a code smell.