Skip to content
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

Fix Incorrect Typing for Margins in the TableConfig Interface Definition #3816

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Maito1794
Copy link

Related to #3815

This change addresses an issue with the typing of the margins property in the TableConfig interface definition.

The config parameter in the table function can include a margins object with the following properties: top, bottom, left, and width. However, the TableConfig interface incorrectly defines margins as a number instead of the correct object type. This inconsistency leads to type-related issues when using the margins property as an object.

See this for the expected value of margins.
See this for the config parameter in table function.

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strengths:

  1. 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.
  2. Logical Grouping:

    • The margins property 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 the margins object.
  3. Readability:

    • The code is cleanly formatted with consistent indentation and spacing, making it easy to read and navigate.
  4. Optional Properties:

    • Using ? for optional properties (printHeaders?, autoSize?, etc.) is correct and aligns with TypeScript best practices.
  5. Type Safety:

    • The use of TypeScript ensures type safety, and the types (boolean, number, etc.) are appropriately chosen for each property.

Areas for Improvement:

  1. width in margins:

    • The width property inside margins might be confusing. Typically, margins define spacing around the content, not the content's width. Consider renaming it to something like contentWidth or moving it outside the margins object 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
  2. Default Values for margins:

    • While fontSize and padding have default values, margins does not. If margins is 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 };
  3. right Margin Missing:

    • The margins object includes top, bottom, and left but omits right. This might lead to inconsistencies in layout. Consider adding a right margin for completeness.
  4. Units Clarification:

    • The documentation mentions "points" for margins, fontSize, and padding. 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.
  5. Validation:

    • If this interface is used in a library or API, consider adding validation logic to ensure values like fontSize and padding are positive numbers, and margins are non-negative.

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;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants