-
Notifications
You must be signed in to change notification settings - Fork 58
INJIWEB-1792,Removed double scrollbars displayed on the view card after clicking view card option. #494
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: sanehema9 <[email protected]>
WalkthroughPDFViewer gained an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@inji-web/src/components/Preview/PDFViewer.tsx`:
- Line 39: The className string in PDFViewer.tsx is using double quotes and a
broken template literal; fix the JSX on the div with ref={containerRef} so it
uses a proper template literal (backticks) and provide a fallback for
props.className (e.g., empty string) when using the nullish coalescing operator;
reference PDFViewerStyles.container and props.className to build the final
className string.
🧹 Nitpick comments (1)
inji-web/src/components/Preview/PDFViewer.tsx (1)
19-32: Consider separating ResizeObserver and URL cleanup into distinct effects.The ResizeObserver logic doesn't depend on
blobUrl, yet it gets torn down and recreated wheneverblobUrlchanges. Separating concerns would avoid unnecessary observer churn:♻️ Suggested refactor
useEffect(() => { if (!containerRef.current) return; const observer = new ResizeObserver(entries => { for (let entry of entries) { setContainerWidth(entry.contentRect.width - containerPadding); } }); observer.observe(containerRef.current); - return () => { - // cleanup the observer and URL - observer.disconnect(); - URL.revokeObjectURL(blobUrl); - }; -}, [blobUrl]); + return () => observer.disconnect(); +}, []); + +useEffect(() => { + return () => URL.revokeObjectURL(blobUrl); +}, [blobUrl]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
inji-web/src/components/Preview/PDFViewer.tsxinji-web/src/components/VC/VCDetailView.tsxinji-web/src/modals/ModalStyles.ts
🧰 Additional context used
🧬 Code graph analysis (1)
inji-web/src/components/VC/VCDetailView.tsx (4)
inji-web/src/modals/Modal.tsx (1)
Modal(31-81)inji-web/src/components/Common/Buttons/ResponsiveIconButtonWithText.tsx (1)
ResponsiveIconButtonWithText(19-75)inji-web/src/components/Common/Icons/DownloadIcon.tsx (1)
DownloadIcon(4-26)inji-web/src/components/Preview/PDFViewer.tsx (1)
PDFViewer(10-63)
🔇 Additional comments (2)
inji-web/src/modals/ModalStyles.ts (1)
12-20: LGTM! Styling changes align with PR objectives.The modal container now properly constrains height with
max-h-[80vh]andoverflow-hidden, while the content container usesoverflow-x-hiddeninstead ofoverflow-y-auto. This correctly delegates vertical scrolling to the PDFViewer component.inji-web/src/components/VC/VCDetailView.tsx (1)
33-36: LGTM! Scroll behavior correctly delegated to PDFViewer.Passing
overflow-y-auto overflow-x-hidden h-fullto PDFViewer ensures the PDF content is the sole scrollable area within the modal, as intended by this PR.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: sanehema9 <[email protected]>
|
@sanehema9 unit tests are failing on the PR, please add unit tests for new code added. Also add proper title to the PR. |
Signed-off-by: sanehema9 <[email protected]>
|



Description :
Issue ticket number and Link
InjiWeb-1792 (https://mosip.atlassian.net/browse/INJIWEB-1792)
Video
INJIWEB-1792.mp4
Summary by CodeRabbit
New Features
Style
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.