Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node missing parent attribute (onNodeClick callback) in v2.x #349

Closed
Joroze opened this issue Jun 11, 2021 · 4 comments
Closed

Node missing parent attribute (onNodeClick callback) in v2.x #349

Joroze opened this issue Jun 11, 2021 · 4 comments
Labels

Comments

@Joroze
Copy link

Joroze commented Jun 11, 2021

Are you reporting a bug, or opening a feature request?

Bug

What is the actual behavior/output?

In version 1.x, the node object that the onClick callback function has contains a "parent" attribute. onNodeClick seems to replace onClick in version 2.x, but the node passed into the callback no longer contains a "parent" attribute.

What is the behavior/output you expect?

onNodeClick should accept the selected node which contains a parent attribute pointing to its parent node (if exists)

Can you consistently reproduce the issue/create a reproduction case (e.g. on https://codesandbox.io)?

Yes

What version of react-d3-tree are you using?

2.x (any 2.x)

@Joroze Joroze changed the title Node missing parent attribute (onNodeClick callback) in v2.0 Node missing parent attribute (onNodeClick callback) in v2.x Jun 11, 2021
bkrem added a commit that referenced this issue Jun 23, 2021
… 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.
@bkrem bkrem added the bug label Jun 23, 2021
@bkrem
Copy link
Owner

bkrem commented Jun 23, 2021

Hi @Joroze,

Thanks for making me aware of this, this was an unintended regression during the huge v2 refactor.

This is one of those refactoring slip-ups where I ask myself how I could've missed this, also because the correct implementation - aligning the type of Node passed to onNode... handlers with the ones already used by onLink...handlers - drastically simplifies the internal handling code 🤦
In retrospect it seems I didn't notice how the underlying structure had subtle changes, while everything looked correct at the code level.

I'm almost done with a fix, but need to put some more due diligence into the release process, since this will need to ship as v3.0.0. The small change needed in the onNode... callback signatures in the top-level API technically makes this a breaking change.

bkrem added a commit that referenced this issue Jun 23, 2021
bkrem added a commit that referenced this issue Jun 24, 2021
… 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.
@bkrem
Copy link
Owner

bkrem commented Jun 24, 2021

Fixed in v3.0.0.

While technically a breaking change due to the adjustment needed in the top-level API, this should be a relatively painless upgrade from v2.

The linked release notes also contain a before/after comparison to hopefully make things easier 🤞

Thanks again for reporting this 👍

@bkrem bkrem closed this as completed Jun 24, 2021
@Joroze
Copy link
Author

Joroze commented Jun 24, 2021

Fixed in v3.0.0.

While technically a breaking change due to the adjustment needed in the top-level API, this should be a relatively painless upgrade from v2.

The linked release notes also contain a before/after comparison to hopefully make things easier 🤞

Thanks again for reporting this 👍

Hi @bkrem, no problem!

How would you feel about the renderCustomNodeElement prop's callback providing the same information such as the parent attribute, so we can also implement our own custom event handling logic leveraging that extra data?

For example, in this CodeSandbox for custom event handlers, nodeDatum is provided (line 18), but it doesn't contain information about if that node has a parent or not. The rd3tProps object should contain parent attribute

A possible solution is to update this line of code to pass this.props.hierarchyPointNode as part of the callback:

return renderCustomNodeElement({ nodeDatum: data, toggleNode: this.handleNodeToggle });

to
return renderCustomNodeElement({ nodeDatum: this.props.hierarchyPointNode, toggleNode: this.handleNodeToggle });

EDIT: I created a pull request to demonstrate. This probably needs to be refactored.

@bkrem
Copy link
Owner

bkrem commented Jun 25, 2021

Additional change for renderCustomNodeElement shipped in v3.1.0 🚀

Thanks for the contribution on this one @Joroze! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants