fix(type): let Graph fitScale properties allow null - #715
Conversation
Sorry, something went wrong.
Update the implementation of the fit method to use more explicit variable name to make the code easier to understand. Also add a new Story to demonstrate the usage of the all zoom and fit methods.
WalkthroughThis pull request updates the Graph class to enhance type safety and code clarity by modifying the minFitScale and maxFitScale properties to accept null values. The fit method has been refactored for improved readability through destructuring and clearer variable names. Additionally, a new Storybook story has been created to showcase the graph component's zoom and fit functionalities with interactive UI controls. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SB as Storybook Story
participant G as Graph Component
U->>SB: Open ZoomAndFit story
SB->>G: Initialize graph container & instance
U->>SB: Click a zoom/fit control button
SB->>G: Invoke corresponding method (e.g., zoomIn, fit)
G-->>SB: Update view with new scale settings
Possibly related PRs
Suggested labelsrefactor Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 Tips ChatThere 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 (.coderabbit.yaml)
Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)packages/html/stories/ZoomAndFit.stories.ts (2)📜 Review detailspackages/core/src/view/Graph.ts (1)1-15: Copyright year should be updated.
The copyright notice states "Copyright 2025-present" which appears to be a year in the future. Consider updating this to reflect the current year or your organization's copyright policy.
-Copyright 2025-present The maxGraph project Contributors +Copyright 2023-present The maxGraph project Contributors
46-47: Consider adding a null/undefined check for args.contextMenu.
The code disables the context menu if args.contextMenu is falsy, but it might be better to explicitly check for null or undefined to avoid unexpected behavior with other falsy values.
- if (!args.contextMenu) InternalEvent.disableContextMenu(container); + if (args.contextMenu === false) InternalEvent.disableContextMenu(container);872-976: Consider refactoring the fit method to use an object parameter.
The fit method has 7 parameters which makes it difficult to use correctly, as noted in the Storybook comments. Consider refactoring to accept a configuration object instead of multiple parameters.
fit( - border: number = this.getBorder(), - keepOrigin = false, - margin = 0, - enabled = true, - ignoreWidth = false, - ignoreHeight = false, - maxHeight: number | null = null + options: { + border?: number; + keepOrigin?: boolean; + margin?: number; + enabled?: boolean; + ignoreWidth?: boolean; + ignoreHeight?: boolean; + maxHeight?: number | null; + } = {} ): number { + const { + border = this.getBorder(), + keepOrigin = false, + margin = 0, + enabled = true, + ignoreWidth = false, + ignoreHeight = false, + maxHeight = null + } = options; // rest of the method implementation...This would make calls clearer, like:
graph.fit({ border: 10, ignoreWidth: true });instead of:
graph.fit(10, false, 0, true, true, false);
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 8385ad5 and a3ab097.
📒 Files selected for processing (2)- packages/core/src/view/Graph.ts (5 hunks)
- packages/html/stories/ZoomAndFit.stories.ts (1 hunks)
packages/html/stories/ZoomAndFit.stories.ts (3)43-49: LGTM! Great setup for the Storybook template.
The Template function initialization is well-structured with proper container setup and graph configuration.
87-113: LGTM! Well-structured graph population.
The batch update pattern is correctly used to efficiently add multiple vertices and edges to the graph in a single operation.
118-119: LGTM! Proper story export.
The story is correctly exported using the bind pattern, which is the recommended approach for Storybook.
packages/core/src/view/Graph.ts (5)361-361: LGTM! Type improvement for scale properties.
Allowing null values for minFitScale and maxFitScale improves type safety and provides more flexibility in controlling the fit behavior.
Also applies to: 367-367
835-871: LGTM! Improved documentation for the fit method.
The updated comments provide clearer information about the parameters and their default values, making the method easier to understand and use correctly.
882-883: LGTM! Improved code readability with destructuring.
Destructuring container and view from this reduces code repetition and improves readability throughout the method.
903-911: LGTM! Better variable naming enhances code clarity.
Replacing s with more descriptive variable names like originalScale and newScale makes the code easier to understand.
924-930: LGTM! Proper handling of nullable scale limits.
The code correctly handles cases where minFitScale or maxFitScale is null, aligning with the updated type definitions.
Sorry, something went wrong.
| addControlButton('Fit Horizontal', function () { | ||
| // This is a pain to use so many parameters when lot of them are the same as default values | ||
| // Consider having a method with a single object | ||
| graph.fit(border, false, 0, true, false, true); | ||
| }); | ||
| addControlButton('Fit Vertical', function () { | ||
| graph.fit(border, false, 0, true, true, false); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chainConsider implementing an object parameter pattern for the fit method.
The code comment correctly identifies a pain point: using multiple positional parameters makes the API harder to use and understand. An object parameter pattern would improve readability and maintenance.
🏁 Script executed:
#!/bin/bash
# Check for any existing implementations of object parameter patterns in the codebase
echo "Searching for existing methods using object parameter patterns..."
rg -A 2 -B 2 "function.*\(\s*{\s*\w+(\s*:\s*\w+)?(\s*,\s*\w+(\s*:\s*\w+)?)*\s*}\s*\)" --type tsLength of output: 407
Below is a revised version of the review comment that reflects our findings:
Actionable Suggestion: Refactor graph.fit Parameters
The current implementation of graph.fit uses several positional parameters, which can be hard to follow and maintain. Our investigation (using our initial regex search and a revised query) did not reveal any existing usage of an object parameter pattern in our TypeScript files. In light of that, it would be beneficial to consider refactoring graph.fit to accept a single object parameter. This change would:
- Improve the readability of the method call.
- Minimize the need to pass many default values.
- Align the API with more modern, maintainable patterns.
Please review this suggestion and verify its consistency with the rest of the codebase.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, this doesn't relate to the purpose of this PR, so I won't manage it for now.
But I keep this in mind for a future improvement as the suggestion is very accurate.
Sorry, something went wrong.
There was a problem hiding this comment.
@tbouffard Understood! Focusing on the current PR's scope is a good approach. Glad to hear the suggestion resonates with you for potential future improvements.
✏️ Learnings added
Learnt from: tbouffard PR: maxGraph/maxGraph#715 File: packages/html/stories/ZoomAndFit.stories.ts:0-0 Timestamp: 2025-03-14T06:01:21.718Z Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Sorry, something went wrong.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)packages/html/stories/ZoomAndFit.stories.ts (1)📜 Review details1-2: Check copyright year
The copyright notice states "2025-present" which appears to be a future date, assuming the current year is 2024. Consider updating to the correct year range.
-Copyright 2025-present The maxGraph project Contributors +Copyright 2024-present The maxGraph project Contributors
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between a3ab097 and 71f2a4d.
📒 Files selected for processing (1)- packages/html/stories/ZoomAndFit.stories.ts (1 hunks)
packages/html/stories/ZoomAndFit.stories.ts (4)43-82: Well-structured control panel
The implementation of the zoom and fit controls is clean and organized. The helper function addControlButton keeps the code DRY and maintainable.
76-78: Consider implementing an object parameter pattern for the fit method
The current implementation of graph.fit uses multiple positional parameters, making the API harder to use and understand as noted in the code comment. An object parameter pattern would improve readability and maintenance.
Example of how this could be refactored:
// Instead of: graph.fit(border, false, 0, true, false, true); // Consider implementing: graph.fit({ border, allowNegativeCoordinates: false, margin: 0, keepOrigin: true, horizontal: true, vertical: false });
87-113: Effective batch update pattern
Using batchUpdate to add multiple graph elements is a good practice for performance optimization. The variety of vertex styles demonstrates good usage of the graph component's capabilities.
115-118: Complete story implementation
The story properly demonstrates zoom and fit functionality with various options, providing a good example for users of the library. This fulfills the PR objective of demonstrating all zoom and fit methods.
Sorry, something went wrong.
Update the implementation of the fit method to use more explicit variable name to make the code easier to understand.
Also add a new Story to demonstrate the usage of the all zoom and fit methods.
Summary by CodeRabbit