fix(type): let Graph fitScale properties allow null#715
Conversation
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/html/stories/ZoomAndFit.stories.ts (2)
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.contextMenuis falsy, but it might be better to explicitly check fornullorundefinedto avoid unexpected behavior with other falsy values.- if (!args.contextMenu) InternalEvent.disableContextMenu(container); + if (args.contextMenu === false) InternalEvent.disableContextMenu(container);packages/core/src/view/Graph.ts (1)
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);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/view/Graph.ts(5 hunks)packages/html/stories/ZoomAndFit.stories.ts(1 hunks)
🔇 Additional comments (8)
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
nullvalues forminFitScaleandmaxFitScaleimproves 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
containerandviewfromthisreduces code repetition and improves readability throughout the method.
903-911: LGTM! Better variable naming enhances code clarity.Replacing
swith more descriptive variable names likeoriginalScaleandnewScalemakes the code easier to understand.
924-930: LGTM! Proper handling of nullable scale limits.The code correctly handles cases where
minFitScaleormaxFitScaleisnull, aligning with the updated type definitions.
| 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 chain
Consider 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.
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/html/stories/ZoomAndFit.stories.ts (1)
1-2: Check copyright yearThe 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/html/stories/ZoomAndFit.stories.ts(1 hunks)
🔇 Additional comments (4)
packages/html/stories/ZoomAndFit.stories.ts (4)
43-82: Well-structured control panelThe implementation of the zoom and fit controls is clean and organized. The helper function
addControlButtonkeeps the code DRY and maintainable.
76-78: Consider implementing an object parameter pattern for the fit methodThe current implementation of
graph.fituses 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 patternUsing
batchUpdateto 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 implementationThe 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.
1 participant
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