Allowing a default size on the graph #11
Conversation
There was a problem hiding this comment.
Sorry – I didn't see the full extent of this PR before reviewing it. I just took a look at the full PR and left some feedback inline.
@rgolea Can you look at the feedback and update this PR with the changes? If you need help to update a PR, take a look at this: https://stackoverflow.com/questions/9790448/how-to-update-a-pull-request
| @@ -51,12 +51,28 @@ graph.connect('peer1', 'peer2') | |||
|
|
|||
| > In graph theory, a directed graph (or digraph) is a graph that is a set of vertices connected by edges, where the edges have a direction associated with them. | |||
|
|
|||
| ### graph = new Graph(rootElem) | |||
| ### graph = new Graph(rootElem, size) | |||
There was a problem hiding this comment.
Optional arguments are indicated in documentation with square brackets like thisgraph = new Graph(rootElem, [size])
|
|
||
| Create a new P2P graph at the root DOM element `rootElem`. In addition to an | ||
| `Element`, a query selector string (like `'.my-cool-element'`) can also be passed | ||
| in. | ||
|
|
||
| Also, you can pass in an object containing the desired width and height. The possible values for both are `auto`, `default` or a number (which represents the number of pixels). Aditionally you can pass in a function that evaluates on resize. |
There was a problem hiding this comment.
I appreciate that you tried to handle all the possible use cases that the user could have! 👍 But I think this is too many different options and we could simplify this.
Which of these options do you actually need for your use case? The only one that webtorrent.io needs is the ability to set a width and height. We can set it to 400 and 250 ourselves, and get rid of those magic constants in p2p-graph.
So, when the size argument is omitted let's just treat that as auto. And when it's specified, let's use the specified values. How does that sound?
That eliminates the auto, default, and function versions.
There was a problem hiding this comment.
The problem here is backwards compatibility and changing it in the webtorrent.io website is not difficult at all, but what happens to the other users that are using this module?
I agree with you that the default and function case should disappear but I added them so each person can decide on it's own use case what will he need in his own project.
Please evaluate this before I start making changes.
There was a problem hiding this comment.
We can publish a new major version, so users will not get the code until they explicitly opt in to it.
| if (typeof root === 'string') root = document.querySelector(root) | ||
| var model = { | ||
| nodes: [], | ||
| links: [] | ||
| } | ||
|
|
||
| if (size) { | ||
| if (size.height && typeof size.height !== 'function' && size.height !== 'default' && size.height !== 'auto' && typeof size.height !== 'number') throw new Error('Height should be a Number or a Function') | ||
| if (size.width && size.width !== 'default' && typeof size.width !== 'function' && size.width !== 'auto' && typeof size.width !== 'number') throw new Error('Width should be a Number of a Function') |
There was a problem hiding this comment.
Long lines are hard to read. It's usually best to hit enter so they don't get this long.
There was a problem hiding this comment.
I will do that. Thank you!
This allows the graph to be set to a custom value