Fixed scroll issue in Shifting and Resource #9475
Conversation
WalkthroughThe pull request modifies the Kanban board component in 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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@rithviknishad I have made the changes. Please Let me know if any further modification needs. |
![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
🧹 Nitpick comments (2)
src/components/Kanban/Board.tsx (2)
18-19: Remove unnecessary commentThe comment "Adjust the path if necessary" appears to be a leftover development note and doesn't provide any value. Consider removing it.
161-161: Extract magic number in height calculationThe height calculation
h-[calc(100vh-250px)]uses a magic number. Consider extracting this value into a constant or CSS variable to improve maintainability and document its purpose.+// Add at the top of the file +const KANBAN_SECTION_HEIGHT_OFFSET = 250; -<ScrollArea className="h-[calc(100vh-250px)]"> +<ScrollArea className={`h-[calc(100vh-${KANBAN_SECTION_HEIGHT_OFFSET}px)]`}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Kanban/Board.tsx(5 hunks)
🔇 Additional comments (3)
src/components/Kanban/Board.tsx (3)
64-64: LGTM: Improved scroll behavior
The updated overflow classes (overflow-x-auto overflow-y-hidden) correctly implement horizontal scrolling while preventing vertical overflow, which is appropriate for a Kanban board layout.
Line range hint 161-186: LGTM: Well-implemented ScrollArea with proper loading states
The ScrollArea implementation effectively solves the scrolling issue while maintaining:
- Proper loading states with skeleton UI
- Clear empty state message with i18n support
- Correct integration with drag and drop functionality
Line range hint 161-186: Verify scroll behavior in different scenarios
While the implementation looks correct, please verify the following scenarios:
- Scrolling behavior with varying numbers of items
- Interaction between board-level and section-level scrolling
- Drag and drop functionality across sections with scroll
✅ Verification successful
Scroll implementation verified and properly integrated
The implementation has been verified through code analysis and shows:
- Proper integration of ScrollArea with drag-and-drop functionality using
@hello-pangea/dnd - Correct scroll event handling for infinite loading (
onBoardReachEnd) - Smooth horizontal scrolling controls with left/right buttons
- Consistent implementation across different board types (Resource, Shifting, Notice boards)
The scroll behavior is well-implemented with:
- Vertical scrolling within sections using
ScrollAreacomponent - Horizontal scrolling at board level with smooth behavior
- Proper cleanup of scroll event listeners
- Scroll-based pagination trigger at 75% scroll threshold
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing scroll-related issues or PRs
gh issue list --search "scroll kanban" --state all
gh pr list --search "scroll kanban" --state all
# Look for other ScrollArea implementations for consistency
rg -l "ScrollArea" --type tsx
Length of output: 1122
Script:
#!/bin/bash
# Search for ScrollArea usage in all TypeScript and TypeScript React files
rg -l "ScrollArea" --type ts --type tsx
# Look for any scroll-related configurations or props in the Kanban components
rg -C 5 "scroll" src/components/Kanban/
# Check for any drag-and-drop related code near ScrollArea usage
rg -C 5 "Draggable|DragDropContext" src/components/Kanban/
# Look for other board implementations that might affect scrolling
fd "Board\.(tsx|ts)$" src/
Length of output: 6745
|
Hey @Jeffrin2005, it’s not fetching beyond the default limit on scroll anymore, which it was doing previously. cc: @rithviknishad |
rithviknishad
added
changes required
and removed
question
@rithviknishad I have committed the changes, but due to some errors, the commits are not showing up. I will close this PR and raise a new one with the updated changes. |
Try to salvage this rather than opening a new PR - since you haven't force pushed yet, you should be able to use git revert to rollback recent changes and then merge/and push once more. |
|
@Jacobjeevan I've encountered some errors while trying to update my PR. Unfortunately, it led to the PR being closed unexpectedly from my Git. While trying to revert and delete the commit, it caused the PR to close. Sorry for stretching the time. I'll close it and raise a new one with the updated changes. |
|
@rithviknishad @Jacobjeevan, can we reopen this PR? |
|
contributors do have permissions to re-open PRs. It maybe disabled if the branch is deleted.
|
Since I've seen a few contributors have this issue where branch is being deleted - not sure what they are doing there 😅 - want to check if this works. @Jeffrin2005 Create a new branch with the same name and SHA of the most recent commit ( |
Jeffrin2005
force-pushed
the
newbranch
branch
from
11687c5 to
8eec748
Compare
|
@rithviknishad @Jacobjeevan, as per the issue raised by @AdityaJ2305, I've updated board.tsx. Now, all sets of cards are being fetched. I've also updated the video. Any errors in this? |
src/components/Kanban/Board.tsx
Outdated
| try { | ||
| const res = await request(options.route, { | ||
| ...options.options, | ||
| query: { | ||
| ...options.options?.query, | ||
| offset: offsetToUse, | ||
| limit: defaultLimit, | ||
| }, | ||
| }); | ||
| const newPages = refresh ? [] : [...pages]; | ||
| const page = Math.floor(offsetToUse / defaultLimit); | ||
| if (res.error) { | ||
| console.error("Error fetching data:", res.error); | ||
| return; |
There was a problem hiding this comment.
let's use useInfiniteQuery
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/Kanban/Board.tsx (2)
95-115: Enhance error handling in fetchPageWhile the error handling is implemented, consider these improvements:
- Use specific error messages that include the actual error details
- Consider showing a user-friendly error notification instead of just logging to console
- The silent failure with empty results might hide issues from users
const fetchPage = async ({ pageParam = 0 }) => { const options = section.fetchOptions(section.id); try { const response = await request(options.route, { ...options.options, query: { ...options.options?.query, offset: pageParam, limit: defaultLimit, }, }); if (response.error) { - throw new Error("Error fetching data"); + throw new Error(`Error fetching data: ${response.error}`); } return response.data as QueryResponse<T>; } catch (error) { - console.error("Error fetching section data:", error); + const errorMessage = error instanceof Error ? error.message : "Unknown error"; + console.error("Error fetching section data:", errorMessage); + // Consider using your notification system here + // showErrorNotification(`Failed to load items: ${errorMessage}`); return { results: [], next: null, count: 0 }; } };
162-172: Consider debouncing the scroll event handlerThe scroll event can fire frequently during scrolling. Consider debouncing the handler to prevent excessive calls to
fetchNextPage.+import { debounce } from "lodash"; + export function KanbanSection<T extends { id: string }>({ // ... existing code ... }) { + const debouncedFetchNext = useRef( + debounce((hasNext: boolean, isFetching: boolean) => { + if (hasNext && !isFetching) { + fetchNextPage(); + } + }, 150) + ).current; + // ... existing code ... - onScroll={(e) => { - const target = e.target as HTMLDivElement; - if ( - target.scrollTop + target.clientHeight >= - target.scrollHeight - 100 - ) { - if (hasNextPage && !isFetchingNextPage) { - fetchNextPage(); - } - } - }} + onScroll={e => { + const target = e.target as HTMLDivElement; + if ( + target.scrollTop + target.clientHeight >= + target.scrollHeight - 100 + ) { + debouncedFetchNext(hasNextPage, isFetchingNextPage); + } + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(2 hunks)src/components/Kanban/Board.tsx(3 hunks)
🔇 Additional comments (1)
src/components/Kanban/Board.tsx (1)
61-61: LGTM! Improved scrolling behavior
The change from overflow-scroll to overflow-x-auto scrollbar-hide improves the scrolling behavior while maintaining a clean UI by hiding the scrollbar.
| @@ -98,6 +98,7 @@ | |||
| "react-i18next": "^15.1.3", | |||
| "react-infinite-scroll-component": "^6.1.0", | |||
| "react-pdf": "^9.1.1", | |||
| "react-query": "^3.39.3", | |||
There was a problem hiding this comment.
Remove redundant react-query dependency
The react-query package is the older version of @tanstack/react-query. Since you're already using @tanstack/react-query (the newer, recommended version), including both packages can lead to conflicts and confusion. Please remove the react-query dependency.
- "react-query": "^3.39.3",
|
@rithviknishad Hi, Used useInfiniteQuery, but there is a slight issue while fetching. A slight delay is occurring, as you can see in this clip. fix.mp4 |
|
That's fine, it's just time spent on fetching the data right. And we are showing the loading state. Expected. |
| try { | ||
| const response = await request(options.route, { | ||
| ...options.options, | ||
| query: { | ||
| ...options.options?.query, | ||
| offset: pageParam, | ||
| limit: defaultLimit, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
can we switch to using callApi or query instead? This will do the handling of errors for you and appropriately show notifications if needed using the queryClient declared globally.
There was a problem hiding this comment.
can we not change the packages and package-lock file in this PR?
Proposed Changes
Fixes Scroll issue in shifting in facilities. #7487
Fixes Infinite Scrolling on Resource Page #9334
Fetches all set of cards
@ohcnetwork/care-fe-code-reviewers
final_scroll.mp4
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor