refactor!: extract Orthogonal and Manhattan connectors #684
Conversation
The connectors are now stored in dedicated files to make the code simpler to read. The remaining utility properties and methods in EdgeStyle were only used by the Orthogonal connector. So, they are now internal. This somewhat reduces the size of maxGraph when it is bundled and minified in applications. BREAKING CHANGES: properties and utility methods previously exposed by `EdgeStyle` are now only internal. The `getRoutePattern` method has been completely removed as it was not being used. Nor was it used in the entire mxGraph and draw.io codebase.
WalkthroughThe pull request restructures the edge routing functionality by refactoring the Changes
Sequence Diagram(s)sequenceDiagram
participant View as Graph View
participant ES as EdgeStyle
participant Manhattan as ManhattanConnector
participant Result as Route Points
View->>ES: Request edge route (source, target)
ES->>Manhattan: Invoke ManhattanConnector
Manhattan->>Manhattan: Execute A* algorithm with grid snapping and obstacle checks
Manhattan-->>ES: Return computed route points
ES-->>View: Render edge using computed route
sequenceDiagram
participant View as Graph View
participant ES as EdgeStyle
participant Orthogonal as OrthogonalConnector
participant Result as Route Points
View->>ES: Request edge route (source, target)
ES->>Orthogonal: Invoke OrthogonalConnector
Orthogonal->>Orthogonal: Calculate jetty sizes and generate waypoints
Orthogonal-->>ES: Return computed route points
ES-->>View: Render edge using computed route
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/core/src/view/style/edge/Orthogonal.ts (3)
58-83: Consider externalizing or documenting complex route patterns.
The hardcodedroutePatternsblock with multiple nested arrays could be clarified by dedicating a separate file or well-documented data structure. This will improve maintainability and reduce cognitive load for future contributors.
92-107: Remove or justify commented-out constants.
There is a block of commented-out constants (e.g.,LEFT_MASK = 32,TOP_MASK = 64) that may cause confusion. If these values are no longer needed, remove them to keep the codebase clean; otherwise, move them to an internal configuration if you plan to use them later.
108-108: Follow through on TODO for magic numbers.
“TODO remove magic numbers” suggests that these references (e.g.,480,512,1024) will be replaced with more expressive constants. Consider extracting them into descriptive named constants.packages/core/src/view/style/edge/Manhattan.ts (1)
210-211: Use optional chaining for clarity.
Adopting optional chaining can simplify the expression and improve readability:- .filter((s) => s.cell && s.cell.isVertex() && !s.cell.isEdge()) + .filter((s) => s.cell?.isVertex() && !s.cell?.isEdge())🧰 Tools
🪛 Biome (1.9.4)
[error] 211-211: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/website/docs/usage/migrate-from-mxgraph.md (2)
374-376: Highlight the Change in VERSION Reference
The moved property note forVERSION—now relocated toconstants.VERSION—is succinct. Consider adding a short note advising developers to update any direct references tomxEdgeStyle.VERSIONin their code.
401-405: Emphasize the Connector-Only API in EdgeStyle
The updated section formxEdgeStylenow explains that the newEdgeStyleclass only includes properties referencing connectors. Since the PR refactors the Orthogonal and Manhattan connectors into dedicated files, a brief example or a link to the detailed global configuration for these connectors would improve clarity. This will help developers understand how to configure the new Orthogonal and Manhattan connectors in their projects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/src/view/style/EdgeStyle.ts(2 hunks)packages/core/src/view/style/config.ts(3 hunks)packages/core/src/view/style/edge/Manhattan.ts(1 hunks)packages/core/src/view/style/edge/Orthogonal.ts(1 hunks)packages/ts-example-without-defaults/vite.config.js(1 hunks)packages/ts-example/vite.config.js(1 hunks)packages/website/docs/usage/migrate-from-mxgraph.md(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/ts-example-without-defaults/vite.config.js
- packages/ts-example/vite.config.js
- packages/core/src/view/style/config.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/core/src/view/style/edge/Manhattan.ts
[error] 211-211: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-2022)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (3)
packages/core/src/view/style/EdgeStyle.ts (3)
22-23: Great import structure for new connectors.
Importing the Manhattan and Orthogonal connectors into separate modules clarifies their usage and supports the new refactoring effort.
145-145: Consistent naming for orthogonal connector.
ReplacingOrthConnectorwithOrthogonalConnectorFunctionfosters clarity of purpose and aligns with the improved naming scheme of dedicated connector files.
152-152: Manhattan connector reference looks good.
The new reference toManhattanConnectorFunctioncontinues the refactoring into dedicated modules. Ensure unit tests cover this integration.
| - `isBrowserSupported` (0.16.0): no longer necessary | ||
| - `setForceIncludes`: the `mxForceIncludes` property has been removed | ||
| - `setLoadResources`: the `mxLoadResources` property has been removed | ||
| - `setLoadStylesheets`: the `mxLoadStylesheets` property has been removed | ||
| - `setResourceExtension`: the `mxResourceExtension` property has been removed | ||
|
|
There was a problem hiding this comment.
Clarify Breaking Changes for Removed Methods
The list of removed methods (e.g. isBrowserSupported, setForceIncludes, setLoadResources, setLoadStylesheets, and setResourceExtension) is clearly documented. It would be helpful to briefly mention any recommended alternatives or to explicitly state that developers should remove any references to these methods in their migration process.
There was a problem hiding this comment.
this is already mentioned: removed because no longer necessary or because the related property has been removed as well.
There was a problem hiding this comment.
@tbouffard You're absolutely right - I apologize for the redundant suggestion. The documentation already clearly explains that isBrowserSupported is no longer necessary, and the other methods were removed due to their corresponding properties being removed. Thank you for the clarification!
The connectors are now stored in dedicated files to make the code simpler to read.
The remaining utility properties and methods in EdgeStyle were only used by the Orthogonal connector. So, they are now
internal.
This somewhat reduces the size of maxGraph when it is bundled and minified in applications.
BREAKING CHANGES: properties and utility methods previously exposed by
EdgeStyleare now only internal.The
getRoutePatternmethod has been completely removed as it was not being used. Nor was it used in the entire mxGraphand draw.io codebase.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores