Conversation
There was a problem hiding this comment.
Pull request overview
Updates the project’s D3 usage and build toolchain as part of moving the demo/docs to D3 v7-compatible behavior.
Changes:
- Switched zoom handling from
d3.eventto the D3 v6+/v7 event argument (e.transform). - Updated demo/docs to load D3 v7 from the CDN.
- Updated build tooling (Rollup + node-resolve) and refreshed
package-lock.json(including lockfile v3).
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/main.js |
Updates zoom handler to D3 v7-style event argument; removes unused d3.tree() call and delete this. |
dist/tidytree.js |
Rebuilt bundle reflecting D3 v7 zoom handler update and minor wrapper change. |
dist/tidytree.min.js |
Rebuilt minified bundle to match current source/bundle behavior. |
app/index.html |
Loads D3 v7 from CDN instead of v5. |
README.md |
Updates example snippet to load D3 v7 from CDN. |
package.json |
Updates Rollup and @rollup/plugin-node-resolve versions. |
package-lock.json |
Upgrades to lockfile v3 and refreshes resolved dependency graph (Rollup 4, D3 patch bump, patristic source). |
Comments suppressed due to low confidence (1)
src/main.js:175
- With D3 v6+ (including v7), event listeners registered via selection.on(...) receive (event, datum) rather than just (datum). This draw() change updates zoom correctly, but the node circle handlers later in this file still use single-arg callbacks (e.g., on('click', d => ...)), which will now receive the DOM event object instead of the node datum and break tooltip/select/contextmenu behavior. Update those handlers to accept (event, d) and pass the datum to trigger(...).
this.zoom = d3.zoom().on("zoom", e => {
let transform = (this.transform = e.transform);
g.attr(
"transform",
`translate(${transform.x},${transform.y}) scale(${transform.k}) rotate(${
this.rotation
},${this.layout === "circular" ? 0 : this.width / 2},${
this.layout === "circular" ? 0 : this.height / 2
})`
);
updateRuler.call(this, transform);
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@rollup/plugin-node-resolve": "^16.0.1", | ||
| "rollup": "^4.41.0" |
There was a problem hiding this comment.
Updating rollup to v4 introduces a Node.js >=18 requirement (see rollup's engines in the lockfile), which will break builds/publishing in Node 16 environments unless the project/tooling is updated accordingly. Consider either adding an explicit "engines" field (and aligning CI/tooling to Node 18+), or pinning rollup to a version compatible with the currently supported Node runtime.
No description provided.