refactor!: move AbstractGraph.fit to FitPlugin
#734
Conversation
WalkthroughThe changes move the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant Graph
participant FitPlugin
User->>Editor: Triggers 'fit' action
Editor->>Graph: getPlugin('fit')
Graph-->>Editor: FitPlugin instance
Editor->>FitPlugin: fit(options)
FitPlugin->>FitPlugin: Compute scale and apply fitting
FitPlugin-->>Editor: scale
Possibly related issues
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core/src/view/plugins/FitPlugin.ts (1)
107-125: Typo in JSDoc@paramdescriptionThe text still says “Optional number” although the parameter is an object:
- * @param options Optional number that specifies the border. + * @param options Optional object to tweak fitting behaviour.Minor, but surfaces in IDE tooltips.
🧹 Nitpick comments (4)
packages/core/src/editor/Editor.ts (2)
62-63: Import path may break tree‑shaking
FitPluginis imported from'../view/plugins', which relies on the barrel file re‑exporting the symbol.
If a future optimisation removes the re‑export (very common when aiming for tree‑shakable bundles), this line will break while the local module path ('../view/plugins/FitPlugin') would still work.-import type { FitPlugin } from '../view/plugins'; +import type { FitPlugin } from '../view/plugins/FitPlugin';Consider importing directly from the concrete module (or keep the barrel but add an explicit re‑export and a unit‑test that guards it).
1031-1033: Graceful degradation if the plugin is absentThe optional‑chaining guards against a missing plugin, but from a UX standpoint silently doing nothing can confuse end‑users (the “Fit” toolbar button appears broken).
Recommend emitting a warning when
getPlugin('fit')returnsundefinedso developers immediately notice a missing plugin registration:- editor.graph.getPlugin<FitPlugin>('fit')?.fit(); +const fit = editor.graph.getPlugin<FitPlugin>('fit'); +if (fit) { + fit.fit(); +} else if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line no-console + console.warn('[Editor] FitPlugin not registered – "fit" action skipped.'); +}(This can be stripped in production builds by dead‑code‑elimination.)
packages/core/src/view/plugins/FitPlugin.ts (2)
21-23:keep2digitsintroduces unnecessary string allocation
Number(value.toFixed(2))allocates an intermediate string each time.
For hot paths you can avoid this with simple arithmetic rounding:-function keep2digits(value: number): number { - return Number(value.toFixed(2)); -} +function keep2digits(value: number): number { + return Math.round(value * 100) / 100; +}Small, but measurable in very large graphs that call
fitCenterfrequently.
179-217: Round applied scale for deterministic re‑fits
fitCenterends withkeep2digits, butfitappliesnewScaledirectly.
Because of floating‑point artefacts successivefit()calls can accumulate tiny translation errors that move the diagram.Consider re‑using
keep2digitshere as well:- view.scaleAndTranslate(newScale, x0, y0); + view.scaleAndTranslate(keep2digits(newScale), x0, y0);Keeps the behaviour symmetrical with
fitCenterand eliminates pixel‑creep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)packages/core/src/editor/Editor.ts(2 hunks)packages/core/src/view/Graph.ts(0 hunks)packages/core/src/view/plugins/FitPlugin.ts(4 hunks)packages/html/stories/OrgChart.stories.js(1 hunks)packages/html/stories/ZoomAndFit.stories.ts(2 hunks)packages/website/docs/usage/migrate-from-mxgraph.md(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/view/Graph.ts
🔇 Additional comments (6)
packages/html/stories/OrgChart.stories.js (1)
287-288: Updated to use the newFitPluginAPI correctly.The code now properly accesses the fit functionality through the plugin system using
graph.getPlugin('fit')?.fit()instead of directly callinggraph.fit(). This aligns with the architectural changes where thefitmethod was moved fromGraphtoFitPlugin.CHANGELOG.md (1)
12-14: Good documentation of breaking changes.The changelog clearly documents that:
- The
Graph.fitmethod has been moved toFitPlugin- The
minFitScaleandmaxFitScaleproperties have also been moved- The method signature has been updated to accept a single parameter object
This provides users with clear information about what has changed and how it impacts their code.
packages/website/docs/usage/migrate-from-mxgraph.md (1)
359-367: Comprehensive migration guidance for the fit functionality.The documentation accurately explains:
- The
fit()method has moved toFitPlugin- It now accepts a single parameter object instead of multiple positional parameters
- The
minFitScaleandmaxFitScaleproperties have also moved toFitPluginThis clear guidance will help users migrate their code to the new API structure.
packages/html/stories/ZoomAndFit.stories.ts (3)
76-77: Good practice: stored plugin reference for reuse.Storing the
FitPluginreference in a variable is more efficient than callinggetPlugin()multiple times, especially when the plugin is used in multiple places.
89-99: Successfully migrated to the new options-based API.The code now correctly uses the new
fitandfitCentermethods with option objects, passing themarginparameter and other configuration options likeignoreHeightandignoreWidth. This approach is more flexible and maintainable compared to positional arguments.
17-17: Import properly updated to include FitPlugin type.The import statement has been updated to include the
FitPlugintype, which is necessary for proper TypeScript typing when getting the plugin withgetPlugin<FitPlugin>('fit').
This change better split responsibilities (reduce the amount of things done directly in Graph) and will improve the tree-shaking in the future. Passing parameters to `Graph.fit` is now simpler. The previous implementation used several positional parameters, which could be hard to follow and maintain. This change: - Improves the readability of the method call. - Minimizes the need to pass many default values. - Aligns the API with more modern, maintainable patterns. In addition, the implementation simplify min/max scale management BREAKING CHANGES: the `AbstractGraph.fit` method moved to `FitPlugin`, as well as the `minFitScale` and `maxFitScale` properties. The method now accepts a single parameter, mainly to minimize the need to pass many default values.
5fad3ad to
b557a6a
Compare
|
Graph.fit to FitPluginAbstractGraph.fit to FitPlugin
This change better split responsibilities (reduce the amount of things done directly in Graph) and improves the tree-shaking for applications that don't use the fit method.
Passing parameters to
Graph.fitis now simpler. The previous implementation used several positional parameters, which could be hard to follow and maintain.This change:
In addition, the implementation simplify min/max scale management.
BREAKING CHANGES: the
AbstractGraph.fitmethod moved toFitPlugin, as well as theminFitScaleandmaxFitScaleproperties.The method now accepts a single parameter, mainly to minimize the need to pass many default values.
Notes
The usage of a single parameter is inspired by #715 (comment)
Impacts on bundled application
For applications not using the FitPlugin and using the Graph class, there is no decrease. This is because FitPlugin is always loaded by the Graph class. This is what we see in js-example and ts-example.
The other examples use BaseGraph which doesn't load FitPlugin by default, only ts-example-selected-features loads it explicitely.
With BaseGraph, when not using FitPlugin, the decrease is about 1 kB
Tasks
Summary by CodeRabbit
New Features
Refactor
Documentation