Skip to content

Commit fa7b69a

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 6074f30 commit fa7b69a

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
@@ -353,11 +353,15 @@ describe('<Tree />', () => {
353353
expect(onClickSpy).toHaveBeenCalledTimes(1);
354354
});
355355

356-
// TODO: Fix expected vs received treeData state
357-
it.skip("clones the clicked node's data & passes it to the onNodeClick callback if defined", () => {
356+
it('clones the `hierarchyPointNode` representation & passes it to the onNodeClick callback if defined', () => {
358357
const onClickSpy = jest.fn();
359358
const mockEvt = { mock: 'event' };
360-
const renderedComponent = mount(<Tree data={mockData} onNodeClick={onClickSpy} />);
359+
360+
const renderedComponent = mount(
361+
// Disable `collapsible` here to avoid side-effects on the underlying tree structure,
362+
// i.e. node's expanding/collapsing onClick.
363+
<Tree data={mockData} onNodeClick={onClickSpy} collapsible={false} />
364+
);
361365

362366
renderedComponent
363367
.find(Node)
@@ -369,7 +373,7 @@ describe('<Tree />', () => {
369373
renderedComponent
370374
.find(Node)
371375
.first()
372-
.prop('data'),
376+
.prop('hierarchyPointNode'),
373377
expect.objectContaining(mockEvt)
374378
);
375379
});
@@ -416,7 +420,7 @@ describe('<Tree />', () => {
416420
expect(onMouseOverSpy).toHaveBeenCalledTimes(0);
417421
});
418422

419-
it("clones the hovered node's data & passes it to the onNodeMouseOver callback if defined", () => {
423+
it('clones the `hierarchyPointNode` representation & passes it to the onNodeMouseOver callback if defined', () => {
420424
const onMouseOverSpy = jest.fn();
421425
const mockEvt = { mock: 'event' };
422426
const renderedComponent = mount(<Tree data={mockData} onNodeMouseOver={onMouseOverSpy} />);
@@ -431,7 +435,7 @@ describe('<Tree />', () => {
431435
renderedComponent
432436
.find(Node)
433437
.first()
434-
.prop('data'),
438+
.prop('hierarchyPointNode'),
435439
expect.objectContaining(mockEvt)
436440
);
437441
});
@@ -478,7 +482,7 @@ describe('<Tree />', () => {
478482
expect(onMouseOutSpy).toHaveBeenCalledTimes(0);
479483
});
480484

481-
it("clones the hovered node's data & passes it to the onNodeMouseOut callback if defined", () => {
485+
it('clones the `hierarchyPointNode` representation & passes it to the onNodeMouseOut callback if defined', () => {
482486
const onMouseOutSpy = jest.fn();
483487
const mockEvt = { mock: 'event' };
484488
const renderedComponent = mount(<Tree data={mockData} onNodeMouseOut={onMouseOutSpy} />);
@@ -493,7 +497,7 @@ describe('<Tree />', () => {
493497
renderedComponent
494498
.find(Node)
495499
.first()
496-
.prop('data'),
500+
.prop('hierarchyPointNode'),
497501
expect.objectContaining(mockEvt)
498502
);
499503
});

‎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)