-
Notifications
You must be signed in to change notification settings - Fork 16
fix(PM-1448): implemented client side infinite scroll #1131
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
@@ -17,6 +17,9 @@ import styles from './SearchResultsPage.module.scss' | |||
const SearchResultsPage: FC = () => { | |||
const [showSkillsModal, setShowSkillsModal] = useState(false) | |||
|
|||
const [currentPage, setCurrentPage] = useState(1) | |||
const itemsPerPage = 10 |
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 making itemsPerPage
a constant outside of the component if it does not change, to avoid re-declaring it on every render.
@@ -25,6 +28,30 @@ const SearchResultsPage: FC = () => { | |||
hasNext, |
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.
Typo in InfiniteTalentMatchesResposne
, it should be InfiniteTalentMatchesResponse
.
const visibleHeight = window.innerHeight | ||
const fullHeight = document.body.scrollHeight | ||
|
||
if (scrollY + visibleHeight >= fullHeight - 100) { |
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 a debounce or throttle mechanism to the scroll event handler to improve performance and prevent excessive function calls during rapid scrolling.
src/apps/talent-search/src/routes/search-results-page/SearchResultsPage.tsx
Outdated
Show resolved
Hide resolved
src/apps/talent-search/src/routes/search-results-page/SearchResultsPage.tsx
Outdated
Show resolved
Hide resolved
const visibleHeight = window.innerHeight | ||
const fullHeight = document.body.scrollHeight | ||
const footerElem = document.getElementById("footer-nav-el"); | ||
const footerHeight = footerElem && footerElem.offsetHeight || 650; |
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 optional chaining when accessing footerElem.offsetHeight
to avoid potential runtime errors if footerElem
is null. For example: const footerHeight = footerElem?.offsetHeight || 650;
const fullHeight = document.body.scrollHeight | ||
const footerElem = document.getElementById("footer-nav-el"); | ||
const footerHeight = footerElem && footerElem.offsetHeight || 650; | ||
if (scrollY + visibleHeight >= fullHeight - footerHeight + 100) { |
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 condition scrollY + visibleHeight >= fullHeight - footerHeight + 100
seems to have a logic change from the original. Ensure that the + 100
adjustment is intentional and correctly accounts for the desired scroll threshold.
src/apps/talent-search/src/routes/search-results-page/SearchResultsPage.tsx
Show resolved
Hide resolved
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.
Looks good
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1448
What's in this PR?