add rounded rectangle #9
Conversation
WalkthroughThis pull request introduces a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant UI as apps/.../test9.html
participant PDFPage
participant Operations
UI->>PDFPage: Call test() with rectangle config (radius: 10.0)
PDFPage->>Operations: Invoke drawRectangle with radius option
Operations->>Operations: If radius > 0, calculate Bezier curves for rounded corners
Operations-->>PDFPage: Return drawing parameters
PDFPage->>UI: Render rounded rectangle
Poem
✨ Finishing Touches
🪧 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 (
|
src/api/operations.ts
Outdated
| radius?: number | PDFNumber; | ||
| }) => { | ||
| if (!options.radius || asNumber(options.radius) <= 0) { | ||
| return [ |
There was a problem hiding this comment.
There are many redundancies, so please write it more efficiently.
There was a problem hiding this comment.
The drawRectangle function is divided into two large blocks due to conditional branching, resulting in significant code duplication.
You can extract the common parts to make the function more DRY.
src/api/operations.ts
Outdated
| const height = asNumber(options.height); | ||
|
|
||
| if (radius > width / 2.0 || radius > height / 2.0) { | ||
| console.warn("radius exceeds the rectangle's width or height"); |
There was a problem hiding this comment.
If the radius exceeds half of the rectangle's width/height, a warning is issued, and an empty array is returned. This causes the rendering to fail, but the error may not be clear.
Please adjust it to the maximum possible radius.
![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 (1)
src/api/operations.ts (1)
277-282: Minor formatting issue in conditional operationsWhile functionally correct, the conditional operations for fill and stroke have inconsistent indentation that doesn't match the rest of the codebase. The
prettier-ignorecomment suggests this is intentional, but you may want to consider standardizing the formatting for better readability.- // prettier-ignore - options.color && options.borderWidth ? fillAndStroke() - : options.color ? fill() - : options.borderColor ? stroke() - : closePath(), + // prettier-ignore + options.color && options.borderWidth ? fillAndStroke() + : options.color ? fill() + : options.borderColor ? stroke() + : closePath(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)src/api/operations.ts(3 hunks)
🔇 Additional comments (5)
README.md (1)
6-9: Well-implemented documentation for the new feature!The documentation clearly describes the rounded rectangle feature, explaining that it uses Bezier curves, requires a positive radius, and handles the case when the radius exceeds half of the width or height. The documentation is consistent with the project's style and provides good information for users.
src/api/operations.ts (4)
228-228: LGTM: New radius parameter properly integratedThe optional
radiusparameter is correctly added to thedrawRectanglefunction options, allowing for rounded corner rectangles while maintaining backward compatibility.
230-260: The implementation correctly handles both standard and rounded rectanglesThe code elegantly handles two scenarios: standard rectangles (when radius is undefined or <= 0) and rounded rectangles (when radius > 0). The approach of using Bezier curves with the KAPPA constant produces smooth rounded corners, and the radius validation ensures the corners look good even when users provide too large values.
245-247: Excellent handling of the radius constraintGood solution for capping the radius to a maximum of half the width or height. This prevents visual artifacts and ensures the rectangle maintains its basic shape while still providing rounded corners.
262-286: Code refactoring improves maintainabilityThe refactoring of the operations into an
opsarray avoids code duplication and makes the function more maintainable. The code structure now cleanly separates the path creation logic from the styling and transformation operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/api/operations.ts (2)
229-286: Consider refactoring to further reduce code duplication.While the implementation is correct, there's still some duplication between the two branches of the conditional. You could potentially extract common operations to reduce duplication further.
export const drawRectangle = (options: { x: number | PDFNumber; y: number | PDFNumber; width: number | PDFNumber; height: number | PDFNumber; borderWidth: number | PDFNumber; color: Color | undefined; borderColor: Color | undefined; rotate: Rotation; xSkew: Rotation; ySkew: Rotation; borderLineCap?: LineCapStyle; borderDashArray?: (number | PDFNumber)[]; borderDashPhase?: number | PDFNumber; graphicsState?: string | PDFName; matrix?: TransformationMatrix; clipSpaces?: Space[]; radius?: number | PDFNumber; }) => { let ops = []; if (!options.radius || asNumber(options.radius) <= 0) { ops = [ moveTo(0, 0), lineTo(0, options.height), lineTo(options.width, options.height), lineTo(options.width, 0), closePath(), ]; } else { let radius = asNumber(options.radius); const width = asNumber(options.width); const height = asNumber(options.height); if (radius > width / 2.0 || radius > height / 2.0) { radius = Math.min(width / 2.0, height / 2.0); } const offset = KAPPA * radius; ops = [ moveTo(0, radius), appendBezierCurve(0, radius - offset, radius - offset, 0, radius, 0), lineTo(width - radius, 0), appendBezierCurve(width - radius + offset, 0, width, radius - offset, width, radius), lineTo(width, height - radius), appendBezierCurve(width, height - radius + offset, width - radius + offset, height, width - radius, height), lineTo(radius, height), appendBezierCurve(radius - offset, height, 0, height - radius + offset, 0, height - radius), closePath(), ]; } return [ pushGraphicsState(), options.graphicsState && setGraphicsState(options.graphicsState), options.color && setFillingColor(options.color), options.borderColor && setStrokingColor(options.borderColor), setLineWidth(options.borderWidth), options.borderLineCap && setLineCap(options.borderLineCap), setDashPattern(options.borderDashArray ?? [], options.borderDashPhase ?? 0), ...(options.clipSpaces ? clipSpaces(options.clipSpaces) : []), options.matrix && concatTransformationMatrix(...options.matrix), translate(options.x, options.y), rotateRadians(toRadians(options.rotate)), skewRadians(toRadians(options.xSkew), toRadians(options.ySkew)), ...ops, // prettier-ignore options.color && options.borderWidth ? fillAndStroke() : options.color ? fill() : options.borderColor ? stroke() : closePath(), popGraphicsState(), ].filter(Boolean) as PDFOperator[]; }
228-260: Consider adding documentation for the radius parameter.Add JSDoc comments to describe the radius parameter, its constraints, and behavior when exceeding limits.
export const drawRectangle = (options: { x: number | PDFNumber; y: number | PDFNumber; width: number | PDFNumber; height: number | PDFNumber; borderWidth: number | PDFNumber; color: Color | undefined; borderColor: Color | undefined; rotate: Rotation; xSkew: Rotation; ySkew: Rotation; borderLineCap?: LineCapStyle; borderDashArray?: (number | PDFNumber)[]; borderDashPhase?: number | PDFNumber; graphicsState?: string | PDFName; matrix?: TransformationMatrix; clipSpaces?: Space[]; + /** + * The radius of the rectangle's corners. + * If not provided or less than or equal to 0, a regular rectangle is drawn. + * If greater than half the width or height, it will be capped at the maximum possible value. + */ radius?: number | PDFNumber; }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/api/operations.ts(3 hunks)
🔇 Additional comments (6)
src/api/operations.ts (6)
228-228: Good API extension!Adding the
radiusoption todrawRectangleis a well-designed enhancement that maintains backward compatibility while providing new functionality.
229-231: Clean refactoring approach.Refactoring to use a local
opsarray before adding it to the final operations is a good approach for improving code readability and maintenance.
232-240: Good handling of default behavior.The conditional check ensures backward compatibility by maintaining the original rectangle drawing behavior when radius is not provided or invalid.
241-247: Excellent validation and adjustment of the radius parameter.The implementation correctly caps the radius to half the width or height, preventing mathematical inconsistencies and rendering issues. This addresses a previous review comment about adjusting to the maximum possible radius rather than failing.
248-259: Well-implemented Bezier curves for rounded corners.The implementation uses the appropriate mathematical approach with the KAPPA constant to create smooth rounded corners. The code is structured clearly with each corner's Bezier curve and connecting lines properly defined.
275-276: Using the ops array in the return statement is clean.This approach reduces duplication and makes the code more maintainable, addressing previous review comments about code redundancy.
What?
Add
radiusoptions todrawRectanglewhich enable us to draw rounded rectangles.Why?
This will help to solve pdfme/pdfme#779
How?
Testing?
Please open the modified test file:
http://localhost:8080/apps/web/test9.html
New Dependencies?
Screenshots
Suggested Reading?
Anything Else?
Checklist
Summary by CodeRabbit
New Features
Documentation