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

fix: add centerNode to demo, fix 'horizontal' Orientation bug #384

Conversation

kalebm1
Copy link
Contributor

@kalebm1 kalebm1 commented Mar 5, 2022

This PR fixes two issues

Add Center Nodes on Click button to demo application

image

I used the same Switch component as the other toggle buttons to create this addition. When clicked, the tree will be centered on a node when it is clicked using the dimensions tree prop.

Fixed the horizontal orientation centering bug

After testing the new centerNode function more thoroughly, I noticed a slight bug with the way we calculate the new x,y coordinates for the horizontal orientation.

Before, we were simply flipping x and y if the orientation was horizontal, but still adding width and height the same. But if we flip x and y, we must also flip the dimension we are adding the original coordinate.

So instead of this for horizontal:

        y = -hierarchyPointNode.x * scale + dimensions.width / 2;
        x = -hierarchyPointNode.y * scale + dimensions.height / 2;

we must do this for horizontal:

        y = -hierarchyPointNode.x * scale + dimensions.height / 2;
        x = -hierarchyPointNode.y * scale + dimensions.width / 2;

This PR takes care of those two issues.

@bkrem
Copy link
Owner

bkrem commented Mar 6, 2022

Add Center Nodes on Click button to demo application

Amazing thank you for already doing this. I ran out of time yesterday but wanted to get the actual changes published so hadn't yet updated the demo with this option.

After testing the new centerNode function more thoroughly, I noticed a slight bug with the way we calculate the new x,y coordinates for the horizontal orientation.

Great catch, thanks for this rapid fix 💯 Weird that I didn't see this during my own testing yesterday, maybe because I had devTools open and that skewed/obscured the miscalculation bug 🤔

@bkrem bkrem changed the base branch from master to fix/horizontal-node-centering-calc March 6, 2022 12:06
@bkrem
Copy link
Owner

bkrem commented Mar 6, 2022

Followup fix for issue #151, PR #381

@bkrem bkrem changed the title Add Center Node to Demo App and Fixed 'horizontal' Orientation bug fix: add centerNode to demo, fix 'horizontal' Orientation bug Mar 6, 2022
@bkrem bkrem merged commit c451295 into bkrem:fix/horizontal-node-centering-calc Mar 6, 2022
bkrem added a commit that referenced this pull request Mar 6, 2022
@bkrem
Copy link
Owner

bkrem commented Mar 6, 2022

Shipped as https://github.com/bkrem/react-d3-tree/releases/tag/v3.3.1 🚢

Thanks again for the rapid followup fix :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants