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

Add option for custom node shapes #27

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

BoZenKhaa
Copy link
Contributor

Adds support for custom node shapes, through new shape parameters. See hello.ipynb for examples:

image

This adds an inconsistency where the shape parameter uses Dict instead of strings in other styling functions. This is because the shape part of the dict is used differently by the javascript than the rest of the parameters. The shapes could be made to work with just strings, but that would require parsing them in javascript. I think it might be better long-term to have all the parameters as Dicts, but that's a breaking change that might be not worth it?

@mykelk mykelk requested a review from zsunberg August 31, 2022 17:46
Copy link
Member

@zsunberg zsunberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BoZenKhaa !

This is awesome! I would like to fix the Dict/String inconsistency that you mention if possible. What is the relationship between style and shape? Can we merge the two? It would probably be more appropriate to have style and link_style be Dicts anyways.

Hopefully it would not be too hard to maintain backwards compatibility by still having a constructor that takes strings.

Let me know what you think.

Another option for an easyish fix would be to only have an element keyword argument which could be circle, rect, etc. and then everything else like r, width, etc. could be controlled with the style keyword.

@BoZenKhaa
Copy link
Contributor Author

BoZenKhaa commented Sep 1, 2022

One problem is that different types of shapes require different styling parameters (r for circle, width and height for square), but those might indeed be passed through style. That is why I added the constants SVG_CRICLE and SVG_SQUARE for ease of use. But it's also true that any styling can be passed through the shape Dict as well.

So another option would be to merge the shape and style into one, which is IMO a clean solution, but then users need to have the js and HTML knowledge to set the style correctly.

Do you have a preference or a better idea of how to address this?

Anyway, I like the idea of converting style and link_style to Dicts and adding parsers for backward compatibility.

@BoZenKhaa
Copy link
Contributor Author

How about keeping the style only as Dict and splatting constants such as SVG_CRICLE as

style=[Dict(SVG_CIRCLE..., "opacity"=>"0.7"), etc.]

to guide users in how to use different shapes?

@zsunberg
Copy link
Member

zsunberg commented Sep 1, 2022

OK, I looked a bit more into how svgs work (https://editsvgcode.com/ is useful). Here are my conclusions:

  1. It seems better to use svg attributes rather than just putting everything in the style attribute - that was a hack to get things working.
  2. We should try as hard as possible to make this a transparent mapping to svg, so that if people find something they want to do with svg, they can do it here
  3. The new D3Tree constructor should have the following keyword arguments:
    • element::Vector{Symbol}, e.g. [:circle, :rect]
    • attributes::Vector{<:Union{Dict{Symbol,String}, NamedTuple}}
    • link_attributes::Vector{<:Union{Dict{Symbol,String}, NamedTuple}}
  4. Defaults can be accomplished by having a default attribute NamedTuple for each element type and using the merge function so that anything the user specifies overrides the default.
  5. Backwards compatibility can be provided with a constructor that just takes in the style keyword argument and does attributes = map(s->(style=s,), style) to put the style in the correct style attribute.

(I am not sure what the tradeoffs for using Symbol vs String are, but it seems like it would be nice to be able to use NamedTuples for performance)

What do you think?

@BoZenKhaa
Copy link
Contributor Author

That sounds reasonable, I will try to rewrite it this way.

@BoZenKhaa BoZenKhaa marked this pull request as draft September 5, 2022 14:09
@BoZenKhaa
Copy link
Contributor Author

BoZenKhaa commented Sep 7, 2022

I tried looking at the SVGs myself, I think it might be better actually to combine the element and its attributes in a single object. The reason is that as I understand it, svgs can contain nested elements. In the future, this would make stuff like this possible as valid node symbols:

<g id="group1" fill="red">
    <rect x="1cm" y="1cm" width="1cm" height="1cm"/>
    <rect x="3cm" y="1cm" width="1cm" height="1cm"/>
</g>

So now, I would either go with

node_svg::Vector{String}

that would just allow any SVG string such as the one above, or use some simple SVG struct:

struct SVG
   element::Symbol  // or String
   attributes::NamedTuple
   // children::Vector{SVG} // as future extension
end

node_svg::Vector{SVG}

The strings can apparently be added with something like

d3.select('#svgtmp').append('div').html('<svg...')

or

d3.select('.container').appendHTML('<div><svg><g><rect width="50" height="50" /></g></svg></div>');

which seems clean, but everything is up to the user then.

Using struct would let us unpack and maybe check the contents of the SVG, but I am not sure that we want to maintain that. Maybe integrating https://github.com/BenLauwens/NativeSVG.jl could then be an option.

I think I prefer the String approach now as that is the most transparent mapping to SVGs we could achieve. What do you think @zsunberg?

I tried to find some good SVG reference to include in the docs, the best I found is https://www.w3.org/TR/SVG/, specifically https://www.w3.org/TR/SVG/struct.html#NewDocument, but that's a bit wordy, do you know of anything better?

@BoZenKhaa
Copy link
Contributor Author

The String approach could also cover the text and tooltip. I think it is reasonable to pass the SVG as a string to let users pass anything they want and to create constructors that provide Julia API.

@zsunberg
Copy link
Member

zsunberg commented Sep 8, 2022

Ideally there would be a nice Julian svg interface like NativeSVG, but that package is not registered. So, I think the string approach is OK. I suggest trying to implement it and seeing if it comes out clean.

@BoZenKhaa
Copy link
Contributor Author

Ok, I will give it a try and we will see what comes out of it.

@BoZenKhaa
Copy link
Contributor Author

The shapes are now entered as SVG strings into node_svg, which are pasted directly into the visualized js node using the .html(svg_string) method. This seems to work nicely, allowing multiple elements to be passed into a node this way.

At the same time, styling through the existing methods (link_style, text, tooltip, style) still works. These are entered into the visualization differently, through other d3 functions.

I now think it might make sense to keep both methods of styling, at least for link_style, text, and tooltip, since they use different mechanisms on the d3 side as well.

With the style, there are some issues to resolve:

  • the Julia style method currently styles the whole node div, not just the shape. That is, setting color in style changes color of the text as well. I think it might make sense for it to only apply to the shape.
  • I am not sure what should take precedence when styling. The current setup seems too complex and confusing:
    • styling entered directly to SVG has precedence over
    • styling entered into CSS, which has precedence over
    • styling entered into the Julia style parameter

Currently, I am inclined to remove style to have users style the SVG string directly in the node_svg. The link_style, text, and tooltip would remain "as is", impossible to be styled unless the users decide to add them manually into node_svg as separate tags.

See the jupyter notebook for how things work in the current version.

@BoZenKhaa
Copy link
Contributor Author

BoZenKhaa commented Oct 4, 2022

Also, instead of constants SVG_CIRCLE and SVG_SQUARE, we could have methods that return styled svg_strings. But I am undecided whether it's worth it to hide the SVG details this way.

These methods could take arguments from style for backward compatibility, which would style only the selected node_svg.

@BoZenKhaa
Copy link
Contributor Author

BoZenKhaa commented Oct 4, 2022

Here is the attempt at providing backward compatibility while allowing custom SVGs. @zsunberg unless you object to this approach, I will clean up, update docs and add tests.

@zsunberg
Copy link
Member

Thanks for the additional work! Conceptually, this seems good to me. I will take another look once the README is updated.

I now think it might make sense to keep both methods of styling, at least for link_style, text, and tooltip, since they use different mechanisms on the d3 side as well.

I think we should only document one way to do it - it is too hard to maintain multiple ways, and it is easy enough for users to construct an svg string. We should only keep the old way for backwards compatibility.

Also, instead of constants SVG_CIRCLE and SVG_SQUARE, we could have methods that return styled svg_strings. But I am undecided whether it's worth it to hide the SVG details this way.

Yes, I agree this is a much better idea than constants! but I agree that this may not be strictly necessary.

@zsunberg
Copy link
Member

At the same time, styling through the existing methods (link_style, text, tooltip, style) still works. These are entered into the visualization differently, through other d3 functions.

Would it make sense just to construct svg strings in julia rather than employing other d3 functions?

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.

2 participants