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

[Bugfix] Fixing Issue With Scrolling Client And Vendor Show Page #2362

Merged
merged 6 commits into from
Feb 18, 2025

Conversation

Civolilah
Copy link
Collaborator

@beganovich @turbo124 The PR fixes the scrolling issue on client and vendor show pages when switching between tabs. Let me know your thoughts.

@Civolilah
Copy link
Collaborator Author

@turbo124 From the discussion on Slack, the way of loading tabs on the Client and Vendor overview pages has been changed, so we will no longer have the "reload" effect. Let me know your thoughts.

@@ -14,7 +14,24 @@ import { useLocation } from 'react-router';
export function ScrollToTop(props: any) {
const location = useLocation();

const pathSegments = location.pathname.split('/');
const id = pathSegments?.[2] || '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you can extract id from the useParams?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beganovich I understand your wondering, but the reason for not having the possibility to extract it from useParams is because ScrollToTop is the parent of the whole <App />, which includes Router as well. Outside of the router concept, react-router-dom is not able to get parameters from the URL, and that is the only reason for it. If we want to achieve getting it with useParams, we would need to apply the ScrollToTop component to each route in the app. However, I think this approach, while not perfect, is not too bad since we just need that one check to see if something is included in the URL. Let me know your thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make sure this works both on hash and regular (browser) route then, please.

Copy link
Collaborator Author

@Civolilah Civolilah Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beganovich I just tested it, the same logic works for both "hash" and "browser" routers. I just did a tiny cleanup when checking if it is a client/vendor show page, but the logic with the ID remains the same.

@beganovich beganovich merged commit 1e03070 into invoiceninja:develop Feb 18, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants