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
Implement <ScrollRestoration /> #5086
Conversation
Hey @FelixMalfait In order to use |
Hey @gitstart-twenty ; switching to createBrowserRouter is the right path. But are you sure the hooks don't work any more? 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 |
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 For providers situated outside the routes, we'll need to find an alternative method for accessing router-related functionality, as the hooks from 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. |
const { pathname } = useLocation(); | ||
const pageTitle = getPageTitleFromPath(pathname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs react-router
context, put under pathless route
@@ -52,17 +76,48 @@ import { SettingsWorkspaceMembers } from '~/pages/settings/SettingsWorkspaceMemb | |||
import { Tasks } from '~/pages/tasks/Tasks'; | |||
import { getPageTitleFromPath } from '~/utils/title-utils'; | |||
|
|||
export const App = () => { | |||
const billing = useRecoilValue(billingState); | |||
const ProvidersThatNeedRouterContext = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need react-router
context, put under pathless route with Outlet
@gitstart-twenty Seems ok for me, do you anything else to move forward ? |
Just needed that green light 🚦 |
Hey @lucasbordeau, Using |
@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 :( It will be nicer once we have this implemented #4914 |
@FelixMalfait This is interesting! Looking into it |
@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 |
Seems like a custom implementation with session storage is the way to go ! |
@gitstart-twenty We talked about the implementation, here's the guidelines :
|
Hey @lucasbordeau
How do we plan on achieving this? Should we limit the amount of scroll possible (maybe not bigger than the screen height)? |
Co-authored-by: v1b3m <vibenjamin6@gmail.com> Co-authored-by: RubensRafael <rubensrafael2@live.com>
We shouldn't scroll if scroll item index > number of items in the table. |
Co-authored-by: v1b3m <vibenjamin6@gmail.com> Co-authored-by: RubensRafael <rubensrafael2@live.com>
Alright! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works fine and covers the use case we have we show page / table navigation !
} else if (state === 'idle' && isDefined(scrollWrapper) && !skip) { | ||
scrollWrapper.scrollTo({ top: scrollPosition }); | ||
} | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't deactivate this rule, this is a code smell.
Co-authored-by: v1b3m <vibenjamin6@gmail.com> Co-authored-by: RubensRafael <rubensrafael2@live.com>
Description
Implement <ScrollRestoration />
Refs
https://github.com/twentyhq/twenty/issues/4357
Demo
scroll.restoration.mp4
Fixes #4357