Fix Incorrect Typing for Margins in the TableConfig Interface Definition#3816
Merged
HackbrettXXX merged 3 commits intoNov 19, 2025
Merged
Conversation
Imran-imtiaz48
left a comment
There was a problem hiding this comment.
Strengths:
-
Clear and Descriptive Documentation:
- Each property has a JSDoc comment explaining its purpose, which is excellent for maintainability and onboarding new developers.
- Default values are explicitly mentioned using
@default, which helps users understand the expected behavior without diving into the implementation.
-
Logical Grouping:
- The
marginsproperty is nested appropriately, and its sub-properties (top,bottom,left,width) are well-documented. This makes it easy to understand the structure and purpose of themarginsobject.
- The
-
Readability:
- The code is cleanly formatted with consistent indentation and spacing, making it easy to read and navigate.
-
Optional Properties:
- Using
?for optional properties (printHeaders?,autoSize?, etc.) is correct and aligns with TypeScript best practices.
- Using
-
Type Safety:
- The use of TypeScript ensures type safety, and the types (
boolean,number, etc.) are appropriately chosen for each property.
- The use of TypeScript ensures type safety, and the types (
Areas for Improvement:
-
widthinmargins:- The
widthproperty insidemarginsmight be confusing. Typically, margins define spacing around the content, not the content's width. Consider renaming it to something likecontentWidthor moving it outside themarginsobject if it represents the width of the printable area.
Suggestion:
margins?: { top: number; bottom: number; left: number; right?: number; // Add right margin for completeness }; contentWidth?: number; // Move width outside margins
- The
-
Default Values for
margins:- While
fontSizeandpaddinghave default values,marginsdoes not. Ifmarginsis optional, consider providing a default object (e.g.,{ top: 0, bottom: 0, left: 0 }) to avoid runtime errors when accessing its properties.
Suggestion:
margins?: { top: number; bottom: number; left: number; right?: number; } = { top: 0, bottom: 0, left: 0 };
- While
-
rightMargin Missing:- The
marginsobject includestop,bottom, andleftbut omitsright. This might lead to inconsistencies in layout. Consider adding arightmargin for completeness.
- The
-
Units Clarification:
- The documentation mentions "points" for
margins,fontSize, andpadding. While this is clear, it might be helpful to specify the unit in the interface name or documentation (e.g.,PrintOptionsInPoints) if this is a strict requirement.
- The documentation mentions "points" for
-
Validation:
- If this interface is used in a library or API, consider adding validation logic to ensure values like
fontSizeandpaddingare positive numbers, andmarginsare non-negative.
- If this interface is used in a library or API, consider adding validation logic to ensure values like
Final Thoughts:
Your PrintOptions interface is well-written and adheres to TypeScript best practices. With a few tweaks (e.g., clarifying width, adding right margin, and providing defaults), it can be even more robust and user-friendly. Great job overall!
Improved Version:
Here’s a slightly improved version based on the suggestions:
interface PrintOptions {
/**
* Whether to print headers or not.
* @default false
*/
printHeaders?: boolean;
/**
* Whether to automatically adjust the size of the content to fit the page.
* @default false
*/
autoSize?: boolean;
/**
* Margins for the printed content (in points).
* @default { top: 0, bottom: 0, left: 0, right: 0 }
*/
margins?: {
/** Top margin in points. */
top: number;
/** Bottom margin in points. */
bottom: number;
/** Left margin in points. */
left: number;
/** Right margin in points. */
right: number;
};
/**
* Width of the content area in points.
*/
contentWidth?: number;
/**
* Font size for the printed content (in points).
* @default 12
*/
fontSize?: number;
/**
* Padding around the content in points.
* @default 10
*/
padding?: number;
}
Collaborator
|
Thanks! |
heikofolkerts
pushed a commit
to heikofolkerts/jsPDF-UA
that referenced
this pull request
Dec 11, 2025
Upstream changes: - Fix Context2d font regex (parallax#3906) - Fix autoPaging in Context2d (parallax#3915) - Fix TypeScript Margins interface (parallax#3816) - Bump GitHub Actions versions (parallax#3907) - Version bump to 3.0.4 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #3815
This change addresses an issue with the typing of the
marginsproperty in theTableConfiginterface definition.The
configparameter in thetablefunction can include amarginsobject with the following properties:top,bottom,left, andwidth. However, theTableConfiginterface incorrectly definesmarginsas anumberinstead of the correct object type. This inconsistency leads to type-related issues when using themarginsproperty as an object.See this for the expected value of
margins.See this for the
configparameter intablefunction.