refactor(stories): remove reference to Graph handler #849
Conversation
This was the former name of `SelectionHandler` in mxGraph. So, rename variables for consistency. Also replace createGraphHandler methods in custom graph implementation as this method no longer exist in AbstractGraph (SelectionHandler is set as a plugin).
|
Caution Review failedThe pull request is closed. WalkthroughThe changes focus on renaming custom selection handler classes and variables from "GraphHandler" to "SelectionHandler" across several story files for clarity and consistency. Additionally, the plugin registration approach is updated to explicitly list plugins, and custom handler logic is refactored to use property overrides instead of subclassing where appropriate. No core logic or control flow is altered. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Graph
participant SelectionHandler
participant Plugins
User->>Graph: Initialize Graph with plugin list
Graph->>Plugins: Register CellEditorHandler, TooltipHandler, etc.
Graph->>SelectionHandler: Use custom MyCustomSelectionHandler
User->>Graph: Interact (e.g., drag, select)
Graph->>SelectionHandler: Delegate selection/drag logic
sequenceDiagram
participant User
participant Graph
participant SelectionHandler
User->>Graph: Drag cell
Graph->>SelectionHandler: Handle drag
SelectionHandler->>SelectionHandler: Custom logic (e.g., redirect drag, set properties)
SelectionHandler-->>Graph: Update selection state
Graph-->>User: Reflect changes in UI
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 error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ 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: 3
🔭 Outside diff range comments (1)
packages/html/stories/Boundary.stories.js (1)
181-186:⚠️ Potential issue
cells.length === 0makes the rest of the condition unreachableAccessing
cells[0]whencells.lengthis0will throw.-return ( - cells.length === 0 && - !cells[0].geometry.relative && - SelectionHandler.prototype.shouldRemoveCellsFromParent.apply(this, arguments) -); +return ( + cells.length > 0 && + !cells[0].geometry.relative && + SelectionHandler.prototype.shouldRemoveCellsFromParent.apply(this, arguments) +);
🧹 Nitpick comments (4)
packages/html/stories/Grid.stories.js (1)
64-66: Guard againstSelectionHandlerbeing undefined
graph.getPlugin('SelectionHandler')can legitimately returnundefinedif the plugin list was customised elsewhere.
A defensive guard avoids a runtimeTypeError.-const selectionHandler = graph.getPlugin('SelectionHandler'); -selectionHandler.scaleGrid = true; +const selectionHandler = graph.getPlugin('SelectionHandler'); +if (selectionHandler) { + selectionHandler.scaleGrid = true; +}packages/html/stories/Labels.stories.js (1)
60-64: Null-safety & property-name sanity-checkSame concern as in
Grid– the plugin may be missing.
Additionally, confirm thatremoveCellsFromParentstill exists on the renamedSelectionHandler; if the library replaced it withshouldRemoveCellsFromParent, this silent assignment will be ignored.-const selectionHandler = graph.getPlugin('SelectionHandler'); -selectionHandler.removeCellsFromParent = false; +const selectionHandler = graph.getPlugin('SelectionHandler'); +if (selectionHandler) { + // NB: property is `removeCellsFromParent` in v4.x, `shouldRemoveCellsFromParent` + // in v5+. Adjust to current API. + (selectionHandler /* as any */).removeCellsFromParent = false; +}packages/html/stories/Groups.stories.js (1)
66-74: Typo in comment – variable renamed but docstring not updatedThe comment still reads “…children selection mode” which referred to the old
graphHandler*naming.
Minor but worth aligning for future searchability.packages/html/stories/DragSource.stories.js (1)
93-95: Pass options via the constructor instead of mutating afterwards
foldingEnabledlives onoptions; setting it after construction is an extra mutation step and easy to forget.- this.options.foldingEnabled = false; - this.recursiveResize = true; + super( + container, + { foldingEnabled: false }, // options + plugins + ); + this.recursiveResize = true; // still needs explicit flagCleaner and makes the intent obvious at the call-site.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/html/stories/Boundary.stories.js(2 hunks)packages/html/stories/Constituent.stories.ts(3 hunks)packages/html/stories/DragSource.stories.js(3 hunks)packages/html/stories/Grid.stories.js(1 hunks)packages/html/stories/Groups.stories.js(2 hunks)packages/html/stories/Labels.stories.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-2022)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (4)
packages/html/stories/Groups.stories.js (1)
91-95: Good catch keeping delegation intactThe wrapper correctly preserves original behaviour via
selectionHandlerIsDelayedSelection.apply.
No issues spotted.packages/html/stories/Boundary.stories.js (1)
154-158: LGTM – simple variable renameNo functional impact; code remains coherent.
packages/html/stories/Constituent.stories.ts (2)
60-71: CustomSelectionHandlerlooks solidOverride cleanly redirects drag starts; good use of
super.
78-92:Details
❌ Incorrect review comment
Possible duplicate plugins – verify
RubberBandHandlerinclusion
getDefaultPlugins()already containsRubberBandHandlerin maxGraph ≥ 0.2.
Including it again may register two instances, causing double event handling.- FitPlugin, - // additional plugin for rubber band selection - RubberBandHandler, + FitPlugin, + // RubberBandHandler is already in the default list for maxGraph ≥0.2 + // RubberBandHandler,
🏁 Script executed:
#!/bin/bash set -e echo "Searching for getDefaultPlugins definition and usages" rg -n "getDefaultPlugins" --context 3Length of output: 32676
No duplicate RubberBandHandler inclusion – explicit inclusion is required
The
getDefaultPlugins()array does not includeRubberBandHandler(per the source and inline docs in RubberBandHandler.ts). Explicitly addingRubberBandHandleris necessary to enable marquee selection. You can leave the code unchanged.Likely an incorrect or invalid review comment.
|
This was the former name of
SelectionHandlerin mxGraph. So, rename variables for consistency.Also replace createGraphHandler methods in custom graph implementation as this method no longer exist in AbstractGraph (SelectionHandler is set as a plugin).
Summary by CodeRabbit