Skip to content

Commit 531f825

Browse files
bkrembkrem
committed
refactor!: aligns onNode handler parameter signatures with onLink handlers (#349)
BREAKING CHANGE: All top-level `onNode` handlers now pass `node: HierarchyPointNode<TreeNodeDatum>` as their first parameter. This change aligns `Node` handlers with those of `Link`, which already return `HierarchyPointNode<TreeNodeDatum>` for their `sourceNode` and `targetNode` parameters. Additionally, this addresses an unintended implicit regression when comparing node handlers to v1 (#349), such as restoring the `parent` node reference in the handlers' `node` parameter.
1 parent 4a7d21f commit 531f825

File tree

5 files changed

+59
-36
lines changed

5 files changed

+59
-36
lines changed

‎src/Node/index.test.js

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,18 @@ describe('<Node />', () => {
1111
},
1212
};
1313

14+
const hierarchyPointNode = {
15+
data,
16+
depth: 1,
17+
height: 0,
18+
parent: null,
19+
x: -200,
20+
y: 200,
21+
};
22+
1423
const mockProps = {
1524
data,
25+
hierarchyPointNode,
1626
nodeSize: {
1727
x: 123,
1828
y: 321,
@@ -123,7 +133,7 @@ describe('<Node />', () => {
123133
expect(onNodeToggleSpy).toHaveBeenCalledWith(data.__rd3t.id);
124134
});
125135

126-
it('handles onNodeClick events and passes its nodeId & event object to handler', () => {
136+
it('handles onNodeClick events and passes its `hierarchyPointNode` representation & event object to handler', () => {
127137
const onClickSpy = jest.fn();
128138
const mockEvt = { mock: 'event' };
129139
const renderedComponent = shallow(
@@ -132,27 +142,36 @@ describe('<Node />', () => {
132142

133143
renderedComponent.find('circle').simulate('click', mockEvt);
134144
expect(onClickSpy).toHaveBeenCalledTimes(1);
135-
expect(onClickSpy).toHaveBeenCalledWith(data.__rd3t.id, expect.objectContaining(mockEvt));
145+
expect(onClickSpy).toHaveBeenCalledWith(
146+
mockProps.hierarchyPointNode,
147+
expect.objectContaining(mockEvt)
148+
);
136149
});
137150

138-
it('handles onNodeMouseOver events and passes its nodeId & event object to handler', () => {
151+
it('handles onNodeMouseOver events and passes its `hierarchyPointNode` representation & event object to handler', () => {
139152
const onMouseOverSpy = jest.fn();
140153
const mockEvt = { mock: 'event' };
141154
const renderedComponent = shallow(<Node {...mockProps} onNodeMouseOver={onMouseOverSpy} />);
142155

143156
renderedComponent.find('circle').simulate('mouseover', mockEvt);
144157
expect(onMouseOverSpy).toHaveBeenCalledTimes(1);
145-
expect(onMouseOverSpy).toHaveBeenCalledWith(data.__rd3t.id, expect.objectContaining(mockEvt));
158+
expect(onMouseOverSpy).toHaveBeenCalledWith(
159+
mockProps.hierarchyPointNode,
160+
expect.objectContaining(mockEvt)
161+
);
146162
});
147163

148-
it('handles onNodeMouseOut events and passes its nodeId & event object to handler', () => {
164+
it('handles onNodeMouseOut events and passes its `hierarchyPointNode` representation & event object to handler', () => {
149165
const onMouseOutSpy = jest.fn();
150166
const mockEvt = { mock: 'event' };
151167
const renderedComponent = shallow(<Node {...mockProps} onNodeMouseOut={onMouseOutSpy} />);
152168

153169
renderedComponent.find('circle').simulate('mouseout', mockEvt);
154170
expect(onMouseOutSpy).toHaveBeenCalledTimes(1);
155-
expect(onMouseOutSpy).toHaveBeenCalledWith(data.__rd3t.id, expect.objectContaining(mockEvt));
171+
expect(onMouseOutSpy).toHaveBeenCalledWith(
172+
mockProps.hierarchyPointNode,
173+
expect.objectContaining(mockEvt)
174+
);
156175
});
157176
});
158177

‎src/Node/index.tsx

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,15 @@ import { select } from 'd3-selection';
55
import { Orientation, Point, TreeNodeDatum, RenderCustomNodeElementFn } from '../types/common';
66
import DefaultNodeElement from './DefaultNodeElement';
77

8-
type NodeEventHandler = (id: string, evt: SyntheticEvent) => void;
8+
type NodeEventHandler = (
9+
hierarchyPointNode: HierarchyPointNode<TreeNodeDatum>,
10+
evt: SyntheticEvent
11+
) => void;
912

1013
type NodeProps = {
1114
data: TreeNodeDatum;
1215
position: Point;
16+
hierarchyPointNode: HierarchyPointNode<TreeNodeDatum>;
1317
parent: HierarchyPointNode<TreeNodeDatum> | null;
1418
nodeClassName: string;
1519
nodeSize: {
@@ -131,15 +135,15 @@ export default class Node extends React.Component<NodeProps, NodeState> {
131135
handleNodeToggle = () => this.props.onNodeToggle(this.props.data.__rd3t.id);
132136

133137
handleOnClick = evt => {
134-
this.props.onNodeClick(this.props.data.__rd3t.id, evt);
138+
this.props.onNodeClick(this.props.hierarchyPointNode, evt);
135139
};
136140

137141
handleOnMouseOver = evt => {
138-
this.props.onNodeMouseOver(this.props.data.__rd3t.id, evt);
142+
this.props.onNodeMouseOver(this.props.hierarchyPointNode, evt);
139143
};
140144

141145
handleOnMouseOut = evt => {
142-
this.props.onNodeMouseOut(this.props.data.__rd3t.id, evt);
146+
this.props.onNodeMouseOut(this.props.hierarchyPointNode, evt);
143147
};
144148

145149
componentWillLeave(done) {

‎src/Tree/index.tsx

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import TransitionGroupWrapper from './TransitionGroupWrapper';
1010
import Node from '../Node';
1111
import Link from '../Link';
1212
import { TreeNodeDatum, Point, RawNodeDatum } from '../types/common';
13-
import { TreeLinkEventCallback, TreeProps } from './types';
13+
import { TreeLinkEventCallback, TreeNodeEventCallback, TreeProps } from './types';
1414
import globalCss from '../globalCss';
1515

1616
type TreeState = {
@@ -303,15 +303,12 @@ class Tree extends React.Component<TreeProps, TreeState> {
303303
/**
304304
* Handles the user-defined `onNodeClick` function.
305305
*/
306-
handleOnNodeClickCb = (nodeId: string, evt: SyntheticEvent) => {
306+
handleOnNodeClickCb: TreeNodeEventCallback = (hierarchyPointNode, evt) => {
307307
const { onNodeClick } = this.props;
308308
if (onNodeClick && typeof onNodeClick === 'function') {
309-
const data = clone(this.state.data);
310-
const matches = this.findNodesById(nodeId, data, []);
311-
const targetNode = matches[0];
312309
// Persist the SyntheticEvent for downstream handling by users.
313310
evt.persist();
314-
onNodeClick(clone(targetNode), evt);
311+
onNodeClick(clone(hierarchyPointNode), evt);
315312
}
316313
};
317314

@@ -330,15 +327,12 @@ class Tree extends React.Component<TreeProps, TreeState> {
330327
/**
331328
* Handles the user-defined `onNodeMouseOver` function.
332329
*/
333-
handleOnNodeMouseOverCb = (nodeId: string, evt: SyntheticEvent) => {
330+
handleOnNodeMouseOverCb: TreeNodeEventCallback = (hierarchyPointNode, evt) => {
334331
const { onNodeMouseOver } = this.props;
335332
if (onNodeMouseOver && typeof onNodeMouseOver === 'function') {
336-
const data = clone(this.state.data);
337-
const matches = this.findNodesById(nodeId, data, []);
338-
const targetNode = matches[0];
339333
// Persist the SyntheticEvent for downstream handling by users.
340334
evt.persist();
341-
onNodeMouseOver(clone(targetNode), evt);
335+
onNodeMouseOver(clone(hierarchyPointNode), evt);
342336
}
343337
};
344338

@@ -357,15 +351,12 @@ class Tree extends React.Component<TreeProps, TreeState> {
357351
/**
358352
* Handles the user-defined `onNodeMouseOut` function.
359353
*/
360-
handleOnNodeMouseOutCb = (nodeId: string, evt: SyntheticEvent) => {
354+
handleOnNodeMouseOutCb: TreeNodeEventCallback = (hierarchyPointNode, evt) => {
361355
const { onNodeMouseOut } = this.props;
362356
if (onNodeMouseOut && typeof onNodeMouseOut === 'function') {
363-
const data = clone(this.state.data);
364-
const matches = this.findNodesById(nodeId, data, []);
365-
const targetNode = matches[0];
366357
// Persist the SyntheticEvent for downstream handling by users.
367358
evt.persist();
368-
onNodeMouseOut(clone(targetNode), evt);
359+
onNodeMouseOut(clone(hierarchyPointNode), evt);
369360
}
370361
};
371362

@@ -509,12 +500,14 @@ class Tree extends React.Component<TreeProps, TreeState> {
509500
);
510501
})}
511502

512-
{nodes.map(({ data, x, y, parent, ...rest }, i) => {
503+
{nodes.map((hierarchyPointNode, i) => {
504+
const { data, x, y, parent } = hierarchyPointNode;
513505
return (
514506
<Node
515507
key={'node-' + i}
516508
data={data}
517509
position={{ x, y }}
510+
hierarchyPointNode={hierarchyPointNode}
518511
parent={parent}
519512
nodeClassName={this.getNodeClassName(parent, data)}
520513
renderCustomNodeElement={renderCustomNodeElement}

‎src/Tree/tests/index.test.js

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -365,11 +365,15 @@ describe('<Tree />', () => {
365365
expect(onClickSpy).toHaveBeenCalledTimes(1);
366366
});
367367

368-
// TODO: Fix expected vs received treeData state
369-
it.skip("clones the clicked node's data & passes it to the onNodeClick callback if defined", () => {
368+
it('clones the `hierarchyPointNode` representation & passes it to the onNodeClick callback if defined', () => {
370369
const onClickSpy = jest.fn();
371370
const mockEvt = { mock: 'event' };
372-
const renderedComponent = mount(<Tree data={mockData} onNodeClick={onClickSpy} />);
371+
372+
const renderedComponent = mount(
373+
// Disable `collapsible` here to avoid side-effects on the underlying tree structure,
374+
// i.e. node's expanding/collapsing onClick.
375+
<Tree data={mockData} onNodeClick={onClickSpy} collapsible={false} />
376+
);
373377

374378
renderedComponent
375379
.find(Node)
@@ -381,7 +385,7 @@ describe('<Tree />', () => {
381385
renderedComponent
382386
.find(Node)
383387
.first()
384-
.prop('data'),
388+
.prop('hierarchyPointNode'),
385389
expect.objectContaining(mockEvt)
386390
);
387391
});
@@ -428,7 +432,7 @@ describe('<Tree />', () => {
428432
expect(onMouseOverSpy).toHaveBeenCalledTimes(0);
429433
});
430434

431-
it("clones the hovered node's data & passes it to the onNodeMouseOver callback if defined", () => {
435+
it('clones the `hierarchyPointNode` representation & passes it to the onNodeMouseOver callback if defined', () => {
432436
const onMouseOverSpy = jest.fn();
433437
const mockEvt = { mock: 'event' };
434438
const renderedComponent = mount(<Tree data={mockData} onNodeMouseOver={onMouseOverSpy} />);
@@ -443,7 +447,7 @@ describe('<Tree />', () => {
443447
renderedComponent
444448
.find(Node)
445449
.first()
446-
.prop('data'),
450+
.prop('hierarchyPointNode'),
447451
expect.objectContaining(mockEvt)
448452
);
449453
});
@@ -490,7 +494,7 @@ describe('<Tree />', () => {
490494
expect(onMouseOutSpy).toHaveBeenCalledTimes(0);
491495
});
492496

493-
it("clones the hovered node's data & passes it to the onNodeMouseOut callback if defined", () => {
497+
it('clones the `hierarchyPointNode` representation & passes it to the onNodeMouseOut callback if defined', () => {
494498
const onMouseOutSpy = jest.fn();
495499
const mockEvt = { mock: 'event' };
496500
const renderedComponent = mount(<Tree data={mockData} onNodeMouseOut={onMouseOutSpy} />);
@@ -505,7 +509,7 @@ describe('<Tree />', () => {
505509
renderedComponent
506510
.find(Node)
507511
.first()
508-
.prop('data'),
512+
.prop('hierarchyPointNode'),
509513
expect.objectContaining(mockEvt)
510514
);
511515
});

‎src/Tree/types.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ import {
1111
TreeNodeDatum,
1212
} from '../types/common';
1313

14-
export type TreeNodeEventCallback = (node: TreeNodeDatum, event: SyntheticEvent) => any;
14+
export type TreeNodeEventCallback = (
15+
node: HierarchyPointNode<TreeNodeDatum>,
16+
event: SyntheticEvent
17+
) => any;
1518

1619
export type TreeLinkEventCallback = (
1720
sourceNode: HierarchyPointNode<TreeNodeDatum>,

0 commit comments

Comments
 (0)