Conditionaly renders pagination component if number of entries is more than results per page #9004
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
WalkthroughThe Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Instead let's update the logic in the Pagination component instead that way this behaviour is common for all usages.
care_fe/src/components/Common/Pagination.tsx
Lines 70 to 73 in 6b57124
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Facility/FacilityStaffList.tsx (2)
122-129: LGTM! Consider extracting the condition for better readability.The conditional rendering logic correctly implements the requirement to show pagination only when needed. The implementation is type-safe with proper null handling.
Consider extracting the condition to a semantic variable name:
+ const shouldShowPagination = (doctorsList?.count ?? 0) > resultsPerPage; - {(doctorsList?.count ?? 0) > resultsPerPage && ( + {shouldShowPagination && ( <Pagination cPage={qParams.page} defaultPerPage={resultsPerPage} data={{ totalCount: doctorsList?.count ?? 0 }} onChange={(page) => updatePage(page)} /> )}
122-129: Consider memoizing the Pagination component to optimize performance.Since the Pagination component's props only depend on a few values (page, resultsPerPage, count), you could optimize re-renders by memoizing it.
Here's how you could implement this:
const MemoizedPagination = useMemo( () => ( <Pagination cPage={qParams.page} defaultPerPage={resultsPerPage} data={{ totalCount: doctorsList?.count ?? 0 }} onChange={(page) => updatePage(page)} /> ), [qParams.page, resultsPerPage, doctorsList?.count, updatePage] ); // Then in your JSX: {(doctorsList?.count ?? 0) > resultsPerPage && MemoizedPagination}
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Common/Pagination.tsx (2)
Line range hint
6-12: Consider simplifying the component props interface.The
dataprop currently only containstotalCount, which could be directly passed as a prop for better clarity and type safety.Consider this improvement:
interface PaginationProps { - data: { totalCount: number }; + totalCount: number; onChange: (page: number, rowsPerPage: number) => void; defaultPerPage: number; cPage: number; className?: string; }Then update the component usage:
-const totalCount = data.totalCount; +const totalCount = props.totalCount;
Line range hint
19-20: Optimize state initialization to prevent unnecessary renders.The
rowsPerPagestate is initialized to 3 but immediately overridden bydefaultPerPagein theuseAbortableEffect. Consider initializing it with thedefaultPerPagevalue directly.- const [rowsPerPage, setRowsPerPage] = useState(3); + const [rowsPerPage, setRowsPerPage] = useState(defaultPerPage);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/Pagination.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Common/Pagination.tsx (1)
71-73: LGTM! The pagination visibility logic is now correct.
The updated condition !totalCount || totalCount < rowsPerPage properly handles all cases:
- Hides pagination when there are no items
- Hides pagination when items fit in a single page
- Correctly handles the edge case when total items exactly equal rows per page
Let's verify the component behavior with different scenarios:
✅ Verification successful
Let me analyze the usage patterns to verify the pagination behavior. I'll search for any test files and check the component's implementation details.
Based on the analysis of the codebase, I can now provide a final response regarding the pagination visibility change:
The pagination visibility logic is correctly implemented and consistent with the codebase's requirements.
The verification confirms:
- The default page sizes across the codebase range from 6 to 36 items (PER_PAGE_LIMIT: 6, RESULTS_PER_PAGE_LIMIT: 14, PAGINATION_LIMIT: 36)
- Multiple components consistently check if
totalCount > PAGINATION_LIMITbefore rendering pagination - The condition
!totalCount || totalCount < rowsPerPageproperly handles all cases:- Zero items (
!totalCount) - Items fitting in a single page (
totalCount < rowsPerPage) - Edge case when total equals page size (correctly hidden)
- Zero items (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases or usage patterns of the Pagination component
# to ensure the new condition works as expected in all scenarios.
# Test: Look for test files related to Pagination
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts Pagination
# Test: Search for Pagination component usage to verify prop patterns
rg -A 3 "<Pagination"
Length of output: 9560
Script:
#!/bin/bash
# Look for default page sizes and pagination limits across the codebase
rg "PAGINATION_LIMIT|PER_PAGE_LIMIT|RESULTS_PER_PAGE|rowsPerPage" -A 2
# Search for Pagination component's implementation details
ast-grep --pattern 'const Pagination = $_' -l typescript
Length of output: 17392
…es/8992/staff-capacity-section-pagination-issue
|
Hi @nihal467 |
…es/8992/staff-capacity-section-pagination-issue
…es/8992/staff-capacity-section-pagination-issue
|
Hi @nihal467, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/components/Facility/FacilityStaffList.tsx (1)
Line range hint
134-139: Improve pagination component rendering logic.Consider these improvements for better user experience:
- Conditionally render pagination only when doctorsList exists and has results
- Add error state handling for doctorsList
- <Pagination - cPage={qParams.page} - defaultPerPage={resultsPerPage} - data={{ totalCount: doctorsList?.count ?? 0 }} - onChange={(page) => updatePage(page)} - /> + {doctorsList?.results?.length > 0 && ( + <Pagination + cPage={qParams.page} + defaultPerPage={resultsPerPage} + data={{ totalCount: doctorsList.count }} + onChange={(page) => updatePage(page)} + /> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityStaffList.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Facility/FacilityStaffList.tsx (1)
25-25:
Verify the intended page size limit.
The change from 15 to 16 items per page seems inconsistent with the PR objectives and previous discussions about showing pagination only when entries exceed 15 items. This might affect the pagination visibility logic.
bodhish
added
needs testing
needs review
and removed
test failed
changes required
labels
|
…es/8992/staff-capacity-section-pagination-issue
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/components/Facility/FacilityStaffList.tsx (3)
36-36: Consider removing redundant state.The
totalDoctorsstate appears redundant sincedoctorsList.countis already available and used for pagination. This creates two sources of truth that could potentially become inconsistent.Consider using
doctorsList?.countdirectly in the UI instead:-const [totalDoctors, setTotalDoctors] = useState(0); // ... -setTotalDoctors(data.count); // ... -<h2 className="mt-2 text-xl font-bold">{totalDoctors}</h2> +<h2 className="mt-2 text-xl font-bold">{doctorsList?.count ?? 0}</h2>
Line range hint
147-152: Address UX concerns from PR comments.Several UX issues were mentioned in the PR comments that need attention:
- Page scrolls to top when changing pages
- Total entries display across all pages
Consider these improvements:
- Maintain scroll position on pagination:
- Wrap the section in a container with fixed height and overflow
- Use scroll restoration or preserve scroll position on page change
- Enhance total entries display:
- Add "Showing X-Y of Z entries" above or below the pagination
Would you like me to provide implementation examples for these improvements?
Based on the findings, I can now provide the final response:
Pagination implementation needs conditional visibility
The Pagination component already has the correct logic to hide when entries are less than or equal to results per page:
if (!totalCount || totalCount <= rowsPerPage) { return null; }However, in FacilityStaffList.tsx, the Pagination component is rendered unconditionally. It should be wrapped in a condition to check if doctorsList exists and has results:
{doctorsList && doctorsList.results.length > 0 && ( <Pagination cPage={qParams.page} defaultPerPage={resultsPerPage} data={{ totalCount: doctorsList?.count ?? 0 }} onChange={(page) => updatePage(page)} /> )}🔗 Analysis chain
Line range hint
1-152: Verify complete implementation of PR objectives.While the basic pagination visibility logic is implemented, there are several items from the PR objectives that need verification or enhancement:
- Pagination visibility for exactly 15 entries
- Behavior when adding the 16th staff type
- Scroll position preservation
- Total entries display
Let's verify the implementation against the PR objectives:
Let me generate additional scripts to verify the pagination implementation:
Let me check the Pagination component implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any pagination-related tests rg -l 'test.*pagination|describe.*pagination' # Search for scroll-related code rg 'scroll|overflow' # Look for staff type addition logic ast-grep --pattern 'function $_(facilityId) { $$$ doctorsList.results.length $$$ }'Length of output: 41456
Script:
#!/bin/bash # Search for pagination component implementation ast-grep --pattern 'function Pagination($props) { $$$ }' # Look for pagination visibility logic in FacilityStaffList ast-grep --pattern 'const FacilityStaffList = ($props) => { $$$ <Pagination $$$ /> $$$ }' # Search for any scroll position management code rg -A 5 "scrollTo|scrollIntoView" src/components/Facility/Length of output: 3921
Script:
#!/bin/bash # Search for Pagination component definition rg -A 20 "export (default )?function Pagination" src/components/Common/ # Check if there's any conditional rendering of Pagination rg -A 5 "{.*Pagination" src/components/Facility/FacilityStaffList.tsx # Look for any totalCount or similar props passed to Pagination rg "totalCount|data\?: \{" src/components/Common/Pagination.tsxLength of output: 541
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Facility/FacilityStaffList.tsx(1 hunks)
🔇 Additional comments (1)
src/components/Facility/FacilityStaffList.tsx (1)
Line range hint 147-152: Verify pagination visibility for edge cases.
Based on the PR comments, there are concerns about pagination visibility when:
- Exactly 15 entries are present
- The 16th staff type is added
Let's verify the pagination visibility logic:
✅ Verification successful
Pagination visibility behavior is correctly implemented
The implementation in Pagination.tsx correctly handles the edge cases mentioned in the PR comments:
- When exactly 15 entries are present (equal to
rowsPerPage), the pagination component returnsnulldue to the condition:
if (!totalCount || totalCount <= rowsPerPage) {
return null;
}- When the 16th staff type is added (exceeding
rowsPerPage), the pagination component will be displayed sincetotalCount > rowsPerPage.
The current implementation matches the expected behavior where pagination should only be visible when there are more items than can fit on a single page.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the Pagination component's null return condition
ast-grep --pattern 'return null'
# Search for any tests related to pagination visibility
rg -l 'test.*pagination.*visibility|describe.*pagination.*visibility'
Length of output: 2758
Script:
#!/bin/bash
# Let's check the Pagination component's implementation
cat src/components/Common/Pagination.tsx
# Let's also check if there are any specific conditions in FacilityStaffList
cat src/components/Facility/FacilityStaffList.tsx
# Search for any tests related to pagination in general
rg -l "test.*pagination|describe.*pagination"
Length of output: 9943
|
Hi @samholics,
|
…es/8992/staff-capacity-section-pagination-issue
…es/8992/staff-capacity-section-pagination-issue
|
LGTM , the remaining things can be taken as separate issue |
|
@mnshrm Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Added logic which makes sure, if number of entries for staff capacity is less than or equal to results per page of that section, only then pagination component is shown, otherwise not
Screenshots
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit