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

Allow passing SVG shape prop (e.g. <rect>, <polygon>) to use as SVG element for nodes #31

Closed
dirodrigues opened this issue Sep 6, 2017 · 16 comments

Comments

@dirodrigues
Copy link

dirodrigues commented Sep 6, 2017

Hi there, first of all awesome work on this library.
I am very very new to using d3, I was wondering if it is possible to render rectangles instead of circles.
I am trying to implement something along the lines of this: http://justincy.github.io/d3-pedigree-examples/basic.html , and would love your help.
Looking into your implementation https://github.com/bkrem/react-d3-tree/blob/master/src/Node/index.js#L91 I can see that you have hard coded circles, would you consider changing this so that whoever is using this library can choose the shape and you could have the circle by default?
Cheers

@bkrem
Copy link
Owner

bkrem commented Sep 7, 2017

Hey @dirodrigues, sorry for the delayed response!

That's a good point, being able to pass SVG shapes as a top-level prop would be way better.

Will jump on this hopefully tomorrow after work or during the weekend, in the meantime feel free to put in your own PR for it if you'd like :)

@dirodrigues
Copy link
Author

Hi @bkrem, I'm going to give it a go today, I'll keep you posted.
Thanks again

@bkrem
Copy link
Owner

bkrem commented Sep 8, 2017

Awesome! There should really be a "Contributing" section in the README (still todo), so let me know if something is unclear/you get stuck, I'll do my best to reply to you ASAP.

@bkrem bkrem changed the title Rectangles instead of circles Allow passing SVG shape prop (e.g. <rect>, <polygon> to use as SVG element for nodes Sep 8, 2017
@bkrem bkrem changed the title Allow passing SVG shape prop (e.g. <rect>, <polygon> to use as SVG element for nodes Allow passing SVG shape prop (e.g. <rect>, <polygon>) to use as SVG element for nodes Sep 8, 2017
@bkrem
Copy link
Owner

bkrem commented Sep 26, 2017

Hey @dirodrigues,

Have some time this week to work on this, but will hold off if you're still looking to publish a PR. Happy to help out on your fork or merge your WIP into a feature branch here if you want to collaborate :)

@iqbalhussain931
Copy link

Hey @bkrem ,
great library awesome work, really appreciate your effort making this awesome library. 👍 However, I need to use some shapes to style the tree making it a little more interactive, waiting for you and @dirodrigues to add that feature. Thanks in advance :)

@bkrem
Copy link
Owner

bkrem commented Oct 3, 2017

Hey @iqbalhussain931,

Thanks, always nice to hear kind words after putting a lot of effort into something like this :)

Yeah I'm keen to get this implemented since I'm sure there will be plenty more people looking to do this in the future.
Will hopefully have this done by the end of the week latest, will start my own branch since I haven't heard back from @dirodrigues but still keen to fast-forward the effort with anything he has to add 👍

Also feel free to open issues for feature requests if you have any more!

@bkrem bkrem added this to the v1.5.0 milestone Oct 3, 2017
@iqbalhussain931
Copy link

iqbalhussain931 commented Oct 6, 2017

Just to get a clear picture, what i want my tree to look like.

demo-tree

@bkrem Thanks in advance.

bkrem added a commit that referenced this issue Oct 6, 2017
Add docs for nodeSvgShape prop
@bkrem
Copy link
Owner

bkrem commented Oct 6, 2017

That's a great use case @iqbalhussain931. It's the level of complexity I'm aiming at getting the library to when it comes to specifying shapes for each individual node.

I've almost finished the nodeSvgShape prop, but this currently only allows setting a shape for all nodes in the tree.
Once this is stable, I'm going to look into implementing shapes via nodeData so each node can specify its shape/colour/text size etc individually, so I'll add it to the v2 roadmap :)

@iqbalhussain931
Copy link

Exactly specifying shapes for each individual node 👍 for now setting a single shape for all nodes will do :)
Thankyou

bkrem added a commit that referenced this issue Oct 6, 2017
Add docs for nodeSvgShape prop
bkrem added a commit that referenced this issue Oct 6, 2017
Add docs for nodeSvgShape prop
@iqbalhussain931
Copy link

@bkrem can you make a build or release of this feature and merge it with master ?

@bkrem
Copy link
Owner

bkrem commented Oct 7, 2017

@iqbalhussain931 Literally about to do that! Wanted to add another prop (textLayout) to make the shapes significantly more useful before releasing v1.5.

Should be up in ~20 mins

@bkrem
Copy link
Owner

bkrem commented Oct 7, 2017

Released in v1.5.0 ✨ ( 👉 Release Notes, updated demo)

Closing this feature request now.
Please open new issue tickets for 1.5.0 if needed, thank you for all your input here, it's been really useful! :)

cc @iqbalhussain931 @dirodrigues @li-xinyang

@bkrem bkrem closed this as completed Oct 7, 2017
@iqbalhussain931
Copy link

iqbalhussain931 commented Oct 7, 2017

@bkrem great work 👍 but i think wouldn't it be better if a node(circle,rect) its self contains the text(name,attribute) in it as i have shown in the example picture, Or as it is now in v1.5.0 node shapes are perfect but text(name,attribute) could be in another shape example rectangle and easily style that rectangle too. what you think would be best?

@bkrem
Copy link
Owner

bkrem commented Oct 7, 2017

@iqbalhussain931 I agree, unfortunately "a contains b" seems pretty complex on an SVG canvas since it's basically just an x-y axis graph without in-built relative layouts.

I'm considering how this can nicely be solved for v2.
In the meantime this is what textLayout is for so the text can be positioned manually to your liking, e.g. check the <rect> example on the demo page.
I'm planning on adding more configurability for the text elements too (e.g. <rect> as a background like you suggested), but I unfortunately don't have anymore time today so it's a todo 👍

@iqbalhussain931
Copy link

yes i have checked the demo its nice. i appreciated you effort on this thank you for this feature. though i have lots of ideas and love to share them with you to make tree more rich easy to use and most importantly interactive for users.

@bkrem
Copy link
Owner

bkrem commented Oct 7, 2017

That would be awesome! Feel free to just open another issue with a feature description and we can do that 😄

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

No branches or pull requests

3 participants