fix[pdfHighlight]: scale pdf down and using scaling to highlight text… #24
Conversation
WalkthroughThe changes in this pull request involve modifications to the CSS and TypeScript files related to the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (8)
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/v0.2.1 #24 +/- ##
=================================================
Coverage ? 53.11%
=================================================
Files ? 36
Lines ? 1751
Branches ? 0
=================================================
Hits ? 930
Misses ? 821
Partials ? 0 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- frontend/src/ee/components/HighlightPdfViewer.module.css (0 hunks)
- frontend/src/ee/components/HighlightPdfViewer.tsx (9 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/ee/components/HighlightPdfViewer.module.css
🧰 Additional context used
🔇 Additional comments (1)
frontend/src/ee/components/HighlightPdfViewer.tsx (1)
145-147: Addition of scaling factors toconstructCoordinatesThe inclusion of
scaleFactorHeightandscaleFactorWidthparameters in theconstructCoordinatesfunction enhances the accuracy of coordinate scaling based on viewport dimensions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
frontend/src/ee/components/HighlightPdfViewer.tsx (3)
151-155: Correct implementation of scaled coordinate calculationsThe updated calculations for x and y coordinates using the scaling factors are correct and align with the function's new parameters. The adjustment for viewHeight in the y-coordinate calculation ensures proper vertical positioning on the scaled PDF.
For improved readability, consider extracting the y-coordinate calculation into a separate variable:
const scaledHeight = height / scaleFactorHeight; const y = viewHeight - (transform[5] / scaleFactorHeight) - scaledHeight;This change would make the calculation easier to understand at a glance.
185-194: Improved error handling and correct scaling factor calculationThe addition of a check for zero values in
viewWidthandviewHeighteffectively prevents division by zero errors, addressing a previous concern. The calculation ofscaleFactorWidthandscaleFactorHeightis correct and essential for accurate highlight positioning.To further improve the code, consider using a guard clause:
if (viewWidth === 0 || viewHeight === 0) { console.error("viewWidth or viewHeight is zero, cannot calculate scaling factors."); return; } const scaleFactorWidth = viewPort.width / viewWidth; const scaleFactorHeight = viewPort.height / viewHeight;This change reduces nesting and makes the code more readable.
359-359: Improve responsiveness and maintain consistent sizingThe use of a fixed width of "600px" for the page container may limit responsiveness on different screen sizes. Consider using a more flexible approach:
- style={{ width: "600px", margin: "0 auto" }} + style={{ maxWidth: "600px", width: "100%", margin: "0 auto" }}This change allows the container to adjust to smaller screen sizes while maintaining a maximum width of 600px on larger screens.
The update to use
viewerRef.current?.offsetWidthfor the Page component's width is a good approach to maintain consistency between the container and the PDF page. However, ensure that this value is reliably set and corresponds to the container's width to avoid potential sizing discrepancies.Also applies to: 366-366
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- frontend/src/ee/components/HighlightPdfViewer.tsx (9 hunks)
🧰 Additional context used
🔇 Additional comments (4)
frontend/src/ee/components/HighlightPdfViewer.tsx (4)
145-147: Improved coordinate calculation with scaling factorsThe addition of
scaleFactorHeightandscaleFactorWidthparameters to theconstructCoordinatesfunction is a positive change. This modification allows for more accurate coordinate calculations, which is crucial for correctly positioning highlights on scaled PDFs.
173-174: Correct viewport initialization and improved readabilityThe initialization of the viewport with a scale of 1 is correct when implementing custom scaling. This ensures that the original dimensions are used as a base for further calculations. The added blank line also improves code readability.
231-233: Consistent use of scaling factors in coordinate calculationsThe
constructCoordinatesfunction is now correctly called with the new scaling factor parameters throughout thehighlightTextInPdffunction. This consistent use of scaling factors ensures that highlight coordinates are accurately calculated and positioned on the scaled PDF.Also applies to: 245-247, 260-262, 275-277
160-161:⚠️ Potential issueIncorrect scaling factor used for width calculation
The width is currently being divided by
scaleFactorHeight, which is inconsistent with the x-coordinate calculation and may lead to distorted highlights. To maintain correct aspect ratios, the width should be divided byscaleFactorWidth.Please update the width calculation as follows:
return { x: x, y: y, - width: width / scaleFactorHeight, + width: width / scaleFactorWidth, height: height / scaleFactorHeight, };This change ensures that both horizontal dimensions (x and width) use the same scaling factor.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- frontend/src/ee/components/HighlightPdfViewer.tsx (9 hunks)
🧰 Additional context used
🔇 Additional comments (4)
frontend/src/ee/components/HighlightPdfViewer.tsx (4)
145-161: LGTM: Improved highlight positioning with scaling factorsThe changes to the
constructCoordinatesfunction look good. The addition ofscaleFactorHeightandscaleFactorWidthparameters and their usage in the calculations improve the accuracy of highlight positioning. This change also addresses the previous issue where an incorrect scaling factor was used for thewidthcalculation.
Line range hint
173-194: LGTM: Improved robustness with scaling factor calculationsThe changes to the
highlightTextInPdffunction are well-implemented. The addition of scaling factor calculations and the check for zero values inviewWidthandviewHeightaddress the potential division by zero issue raised in a previous review. This improves the overall robustness of the function.
Line range hint
213-233: LGTM: Enhanced overlap detection and consistent use of scaling factorsThe changes to the overlap detection logic are well-implemented. The additional overlap check improves the accuracy of text matching. The
constructCoordinatesfunction call has been correctly updated to include the new scaling factor parameters, maintaining consistency with the earlier changes.
Line range hint
245-277: LGTM: Consistent application of scaling factors in highlight calculationsThe changes to the highlight coordinate calculations are well-implemented. All instances of the
constructCoordinatesfunction calls have been correctly updated to include the new scaling factor parameters. This ensures consistent and accurate highlight positioning across different scenarios.
#24) * fix[pdfHighlight]: scale pdf down and using scaling to highlight text in pdf * fix[pdfViewer]: requested code refactory * fix[pdfHighlight]: extras leftovers * fix[pdfHighlight]: refactor and add highlights in pdf
… in pdf
Summary by CodeRabbit
New Features
Bug Fixes
Style