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

Exposing getNodeRect #20

Merged

Conversation

cuginoAle
Copy link
Collaborator

@cuginoAle cuginoAle commented Oct 24, 2018

With this PR d3-fg will expose the getNodeRect public method.
It accepts the node data object and it returns an object containing the node position and size (or null if no node is found).
The returned object has now the following shape:
{x, y, width, height}

This has been updated to match the object shape returned by getClientBoundingRect.

This method is useful when we need to overlap external components over a particular frame, like the following screenshot demonstrates:

screenshot 2018-10-24 at 21 24 05

@goto-bus-stop
Copy link
Contributor

data.id is a Flame-specific thing, 0x doesn't have it. (The distinction is definitely not very clear…) Could we pass in the node.data object instead? Existing APIs use that, like the chart.zoom(d) method.

We should document the properties that a d3-fg data object always has in the readme so we don't have to rely on people knowing and noticing which things are specific to 0x and Flame. I think those would be the same properties more or less as returned by the ticksToTree method in 0x. Flame adds id and a few more, and 0x has IDs but it uses a Map to store data/id pairs instead of adding an id property.

@cuginoAle
Copy link
Collaborator Author

@goto-bus-stop Good to know, thanks for clarifying this!
Sorted now.

@AlanSl
Copy link
Contributor

AlanSl commented Oct 25, 2018

LGTM

@mcollina mcollina merged commit 88c8f69 into davidmarkclements:master Oct 25, 2018
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.

4 participants