feat: let "fit center" with the new FitPlugin
#733
Conversation
Introduce `FitPlugin` which is intended to contain all fit methods in the future. The “fit center” implementation is inspired by the old example provided in the `Graph.fit` method. It has been updated to improve margin management and to limit rounding impacts, particularly when the method is called several times in succession. The ts-example and the story now use the new method to demonstrate its use and operation in different contexts.
WalkthroughThis pull request updates multiple components of the graph view system. The test case is modified to expect an extra default plugin. Documentation in the Graph class has been updated to reference a new fit functionality now implemented in the newly introduced FitPlugin. The FitPlugin itself is added to the default plugins list, and its functionality is integrated into storybook examples and a TypeScript example. Additionally, minor configuration changes include updating the TypeDoc category order in the tsconfig file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Button as "Fit Center Button"
participant Main as "main.ts"
participant Graph as "Graph Instance"
participant FitPlugin as "FitPlugin"
User->>Button: Click "Fit Center"
Button->>Main: Trigger event listener
Main->>Graph: Retrieve FitPlugin
Graph->>FitPlugin: Call fitCenter({ margin: 20 })
FitPlugin->>Graph: Adjust scaling and center graph
Graph-->>Main: Updated graph state
Main-->>User: Display centered graph
Possibly related PRs
Suggested labels
✨ Finishing Touches
🪧 TipsChatThere 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ts-example/index.html (1)
21-24: Controls implementation looks good.The HTML structure for the zoom and fit center buttons is clean and correctly uses the new
.controlsclass for styling. The buttons have appropriate IDs for JavaScript targeting.Consider adding ARIA attributes to improve accessibility:
- <button id="reset-zoom">Reset Zoom</button> - <button id="fit-center">Fit Center</button> + <button id="reset-zoom" aria-label="Reset graph zoom to default">Reset Zoom</button> + <button id="fit-center" aria-label="Fit graph to center of view">Fit Center</button>packages/core/__tests__/view/plugins/index.test.ts (1)
31-31: Test correctly updated for new plugin count.The test expectation has been properly updated to reflect the addition of the FitPlugin to the default plugins array.
Consider enhancing this test to verify not just the count but also the presence of specific plugins including the new FitPlugin:
test('returns an array with the correct length', () => { const plugins = getDefaultPlugins(); // detect any changes in default plugins, order does not matter expect(plugins).toHaveLength(8); + // Verify FitPlugin is included (you'll need to import the actual plugin constructor) + expect(plugins.some(plugin => plugin.name === 'fitPlugin')).toBe(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/core/__tests__/view/plugins/index.test.ts(1 hunks)packages/core/src/view/Graph.ts(1 hunks)packages/core/src/view/plugins/FitPlugin.ts(1 hunks)packages/core/src/view/plugins/index.ts(2 hunks)packages/core/tsconfig.json(1 hunks)packages/html/stories/ZoomAndFit.stories.ts(4 hunks)packages/ts-example/index.html(1 hunks)packages/ts-example/src/main.ts(2 hunks)packages/ts-example/src/style.css(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/core/src/view/plugins/index.ts (1)
packages/core/src/view/plugins/FitPlugin.ts (1)
FitPlugin(44-110)
packages/core/src/view/plugins/FitPlugin.ts (1)
packages/core/src/view/Graph.ts (1)
Graph(1488-1488)
packages/html/stories/ZoomAndFit.stories.ts (1)
packages/core/src/view/plugins/FitPlugin.ts (1)
FitPlugin(44-110)
🔇 Additional comments (24)
packages/ts-example/src/style.css (1)
43-45: Good addition of the controls class.The
.controlsclass with a margin-top of .75rem provides appropriate spacing for the newly added control buttons in the HTML file. This styling is clean and follows the existing pattern in the CSS file.packages/core/tsconfig.json (1)
31-31: Appropriate documentation category addition.Adding "Navigation" as a category in the TypeDoc configuration is appropriate for organizing the new FitPlugin functionality. The placement in the category order is logical.
packages/core/src/view/plugins/index.ts (3)
25-25: Appropriate import of the new FitPlugin.The import statement correctly brings in the FitPlugin class from its module file.
27-28: Good practice with barrel exports.Exporting all contents from the FitPlugin module makes the plugin components available through the main barrel file, which follows good module organization patterns.
46-46: Correct integration of FitPlugin into default plugins.The FitPlugin is properly added to the array returned by getDefaultPlugins, making it available by default when initializing a Graph instance.
packages/ts-example/src/main.ts (3)
21-21: Correct type import.The FitPlugin type is appropriately imported to enable type checking and IDE support.
116-116: Good enhancement to return the graph instance.Modifying the function to return the created graph instance improves reusability and allows for further interaction with the graph after initialization.
126-132: Well-implemented demonstration of FitPlugin usage.The event listeners for the control buttons provide a clear example of how to use the FitPlugin in practice. The implementation correctly retrieves the plugin using the type-safe
getPlugin<FitPlugin>('fit')method and calls itsfitCentermethod with appropriate parameters.packages/core/src/view/Graph.ts (1)
847-847: Documentation properly updated to reference the new plugin method.The documentation now correctly directs users to use the
FitPlugin.fitCentermethod instead of providing a manual implementation. This encourages the use of the dedicated plugin for fitting functionality.packages/core/src/view/plugins/FitPlugin.ts (6)
21-23: Good utility function for precision control.The
keep2digitsfunction handles rounding to two decimal places, which helps minimize floating-point precision issues. This is especially important for visual consistency when the fitCenter method is called multiple times.
30-36: Well-defined options type with proper JSDoc.The
FitCenterOptionstype is properly defined with clear JSDoc comments that explain the purpose and default value of each option. This improves developer experience when using the API.
44-52: Clean class declaration with appropriate configuration.The FitPlugin class has a clear plugin identifier and configurable
maxFitScaleproperty with a default value. The JSDoc comments provide good context about the purpose and usage of the class and its properties.
53-59: Well-designed constructor.The constructor correctly takes a Graph instance and stores it as a private readonly property, following good practices for dependency injection and immutability.
60-104: Well-implemented fitCenter method with rounding error prevention.The fitCenter method is thoughtfully implemented to address the concerns mentioned in the PR description:
- It properly handles margins
- It calculates appropriate scale while respecting maxFitScale
- It includes specific measures to minimize rounding errors when called multiple times
- It uses Math.floor for translations to ensure consistent integer values
The comment on line 67 also provides valuable context by referring to the original implementation that inspired this method.
106-109: Empty but necessary lifecycle method.The
onDestroymethod is correctly implemented as a no-op method since this plugin doesn't require cleanup. Including this method ensures the plugin properly implements the GraphPlugin interface.packages/html/stories/ZoomAndFit.stories.ts (9)
17-17: Good addition of FitPlugin type importThe import has been properly updated to include the FitPlugin type, which is necessary for typechecking when using the plugin's methods. Using the 'type' prefix in the import is good practice as it indicates this is only used for TypeScript type checking.
35-42: Good configurability additions for testingThe new boolean properties
graphWithLargeHeightandcontainerWithScrollbarprovide good flexibility for testing the fit functionality in different scenarios. These properties will help demonstrate how the FitPlugin handles different graph sizes and container configurations.
48-49: Default values are appropriateSetting
graphWithLargeHeightto false by default creates a more compact initial view, while keepingcontainerWithScrollbardisabled by default provides a cleaner initial appearance. These defaults make sense for a demo story.
56-59: Good conditional styling implementationThe conditional application of the scrollbar style is clean and straightforward. This will help demonstrate how the fit functionality behaves when the container has scrollbars.
85-85: Appropriate variable renaming from border to marginRenaming from
bordertomarginaligns with the terminology used in the FitPlugin API, creating consistency between the UI controls and the underlying implementation.
87-88: Updated fit method signatureThe updated call to
graph.fit()now usesundefinedas the first parameter, following the API changes in the Graph class where fit functionality is now delegated to the FitPlugin.
89-91: Good implementation of Fit Center functionalityThe new Fit Center button correctly uses the plugin system to access the FitPlugin and call its
fitCentermethod. The optional chaining operator (?.) is a good defensive programming practice in case the plugin isn't available.
95-95: Consistent updates to all fit method callsAll calls to
graph.fit()have been consistently updated to useundefinedfor the first parameter and themarginvariable. This ensures uniform behavior across all fit operations.Also applies to: 98-98
123-123: Dynamic vertex positioning for testingThe vertex position is now dynamically set based on the
graphWithLargeHeightproperty. This is a good approach for testing how the fit functionality handles different graph layouts and aspect ratios.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/view/plugins/FitPlugin.ts (3)
20-22: Consider making the utility function more reusable.The
keep2digitsfunction is a useful utility for handling floating-point precision. Consider either:
- Moving it to a shared utility module if similar rounding is needed elsewhere
- Making it more configurable by accepting the number of decimal places as a parameter
-function keep2digits(value: number): number { - return Number(value.toFixed(2)); +function keepNDigits(value: number, digits: number = 2): number { + return Number(value.toFixed(digits)); }
50-50: Consider documenting the behavior when maxFitScale is null.The
maxFitScaleproperty can be set tonull, but it might not be immediately clear to users what this means. Consider adding a bit more detail to the JSDoc comment./** * Specifies the maximum scale to be applied during fit operations. Set this to `null` to allow any value. + * When set to `null`, the scale will only be limited by the container dimensions. * @default 8 */
105-108: Empty lifecycle method implementation.The
onDestroymethod is a no-op, which is fine if it's required by the interface but not needed for this plugin. Consider adding a bit more context in the comment about why it's not needed.-/** Do nothing here. */ +/** + * Required by GraphPlugin interface but not needed for this plugin + * as there are no resources to clean up. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/view/plugins/FitPlugin.ts(1 hunks)packages/ts-example/index.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ts-example/index.html
🧰 Additional context used
🧬 Code Definitions (1)
packages/core/src/view/plugins/FitPlugin.ts (1)
packages/core/src/view/Graph.ts (1)
Graph(1488-1488)
🔇 Additional comments (4)
packages/core/src/view/plugins/FitPlugin.ts (4)
29-35: The type definition looks good.The
FitCenterOptionstype is well-documented with appropriate JSDoc comments and provides a clear interface for customizing the fitting behavior.
43-57: Well-structured plugin class with clear documentation.The plugin implementation follows good practices:
- Implements the required interface
- Uses a static
pluginIdfor identification- Has configurable properties with documented defaults
- Uses dependency injection via constructor parameter
67-67:✅ Verification successful
Verify margin application consistency.
The margin is applied by subtracting
2 * marginfrom both width and height, but it's not clear if this correctly applies the margin on all sides.
🏁 Script executed:
#!/bin/bash # Look for similar margin handling in the codebase rg -A 3 -B 3 "margin.*clientWidth|clientWidth.*margin" --type ts packages/core/src/Length of output: 650
Margin Application Verified
After verification, the margin is correctly applied in
FitPlugin.tsby subtracting2 * marginfrom both the container's width and height. This approach properly accounts for margins on both sides (left/right and top/bottom), and no inconsistencies were found in the codebase.
65-103:❓ Verification inconclusive
Good implementation with careful handling of rounding issues.
The
fitCentermethod is well-implemented with:
- Clear documentation and parameter validation
- Proper margin handling
- Careful consideration of rounding errors
- Scale limitation based on configurable max scale
- Efficient calculation of translation values
However, there are a few considerations:
Please verify how the method behaves in edge cases:
- What happens when the graph is empty or has zero width/height?
- Does the method handle negative bounds correctly?
🏁 Script executed:
#!/bin/bash # Look for any handling of empty graphs or bounds in the codebase rg -A 3 -B 3 "bounds.width\s*===?\s*0|width\s*===?\s*0|emptyGraph|empty graph" --type ts packages/core/src/Length of output: 2271
Edge Cases Verification for
fitCenterMethod – Please Confirm BehaviorThe current implementation is solid in handling rounding precision and scaling limits. However, please verify the following:
Empty Graph/Zero Dimensions:
Ifthis.graph.getGraphBounds()returns a rectangle with zero width/height (e.g., an empty graph), the divisions in the calculation could lead to unexpected results (e.g., division by zero or infinite scale). Confirm thatgetGraphBoundsor the calling context guarantees non-zero dimensions—or consider adding guards against zero values.Negative Bounds:
While negative bounds are computed arithmetically (e.g., throughbounds.xandbounds.y), ensure that the resultant translation values are correct and render as expected.Please review these edge cases in a running environment to make sure that the behavior meets the intended design.
1 participant
Introduce
FitPluginwhich is intended to contain all fit methods in the future.The “fit center” implementation is inspired by the old example provided in the
Graph.fitmethod.It has been updated to improve margin management and to limit rounding impacts, particularly when the method is called several times in succession.
The ts-example and the story now use the new method to demonstrate its use and operation in different contexts.
Notes
Tested with Firefox 136.0.1 and Chrome 134.0.6998.88 (64 bits) on Ubuntu 22.04
Screenshots
PR_733_story_fit_center.mp4
Summary by CodeRabbit
New Features
Documentation
FitPlugin.Style