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

Narrower type to the "type" field in NotificationsOptions #66

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

Lirik-Keltran
Copy link
Contributor

Its simplifies notification configuration

@@ -3,7 +3,7 @@ export interface NotificationsOptions {
id?: number;
title?: string;
text?: string;
type?: string;
type?: "warn" | "success" | "error";
group?: string;
duration?: number;
speed?: number;
Copy link

Choose a reason for hiding this comment

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

Code Review:

  1. Change Made:

    • The type property in the NotificationsOptions interface has been changed from a generic string type to a specific union type "warn" | "success" | "error". This is a good change as it restricts the possible values that type can have, making the code more type-safe and reducing the chance of bugs related to incorrect type values.
  2. Positive Points:

    • Using a union type with specific allowed values for type improves code clarity and helps developers understand what values are acceptable without having to refer to documentation or implementation details.
    • Explicitly defining allowed values reduces the risk of unintentionally passing incorrect types or values, which can help catch errors during development.
  3. Improvement Suggestions:

    • It's a minor improvement but consider adding comments to describe each property in the NotificationsOptions interface. It can provide quick insights into the purpose of each property.
    • Add documentation/comments to explain the purpose of the speed, duration, and other properties, especially if their usage might not be immediately obvious.
  4. Bug Risks:

    • The code change itself does not introduce any bug risks; in fact, it reduces some potential risks by enforcing stricter typing for the type property. However, remember to update all places where NotificationsOptions interface is used to reflect this change to prevent any mismatch in type expectations.
  5. Overall Thoughts:

    • The change made in the code patch is a step in the right direction, enhancing code quality and maintainability by leveraging TypeScript's type system effectively.

Summary:

The code change you provided is beneficial as it enforces stricter type checking for the type property in the NotificationsOptions interface, reducing potential errors related to incorrect type values. Consider adding comments for better code comprehension and ensure consistency by updating relevant parts of the codebase to align with this change.

@kyvg kyvg merged commit 34b0221 into kyvg:master Mar 19, 2024
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