-
Couldn't load subscription status.
- Fork 2.3k
fix: preserve scroll position when switching tabs in settings #7587
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
Conversation
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.
Thank you for your contribution! I've reviewed the changes and found that the implementation successfully addresses the scroll position preservation issue. However, there are a few suggestions that could improve the code quality and maintainability.
| </div> | ||
| ) | ||
| } | ||
| }) |
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.
The forwardRef component should have a display name for better debugging and React DevTools support. Consider adding:
| }) | |
| TabContent.displayName = 'TabContent' |
right after the component definition.
| const scrollPositions = useRef<Record<SectionName, number>>( | ||
| Object.fromEntries(sectionNames.map((s) => [s, 0])) as Record<SectionName, number>, | ||
| ) | ||
| const contentRef = useRef<HTMLDivElement | null>(null) |
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.
Consider using a more descriptive variable name like tabContentScrollRef instead of contentRef to better indicate its specific purpose of tracking scroll positions.
| ) | ||
|
|
||
| const scrollPositions = useRef<Record<SectionName, number>>( | ||
| Object.fromEntries(sectionNames.map((s) => [s, 0])) as Record<SectionName, number>, |
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.
The scroll positions are stored indefinitely in the ref. While not critical for a small number of tabs, consider if any cleanup is needed when the component unmounts to prevent potential memory leaks in long-running sessions.
| contentRef.current.scrollTop = scrollPositions.current[activeTab] ?? 0 | ||
| } | ||
| }) | ||
| }, [activeTab]) |
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.
Consider adding test coverage for the scroll position preservation functionality. The PR checklist indicates "Testing" is unchecked. Adding tests would help ensure this feature works correctly and prevent regressions. You could test:
- Scroll position is saved when switching tabs
- Scroll position is restored when returning to a tab
- Initial scroll position is 0 for unvisited tabs
…: name TabContent for clearer DevTools
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.
Thank you @DC-Dancao!
Description
This PR implements independent scroll positions for each tab within the settings view, enhancing user experience. Previously, the scroll position was shared across all tabs, causing incorrect scrollbar behavior when switching between tabs of different lengths. Now, the view remembers each tab's scroll position, restoring it when the user navigates back.
Changes
src/components/Tab.tsx: Enabled ref forwarding for the TabContent component to allow parent access.
src/views/SettingsView.tsx: Implemented logic to save and restore individual scroll positions when switching tabs.
Pre-Submission Checklist
Important
Implements independent scroll positions for each tab in settings, enhancing user experience by restoring positions when switching tabs.
SettingsView.tsx, restoring them when switching tabs.TabContentinTab.tsxto allow parent access.useRefanduseEffectinSettingsView.tsxto manage scroll positions.handleTabChangeanduseEffectinSettingsView.tsxupdated to save and restore scroll positions.This description was created by
for ad07bf4. You can customize this summary. It will automatically update as commits are pushed.