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

Allowing a default size on the graph #11

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

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

Optional arguments are indicated in documentation with square brackets like thisgraph = new Graph(rootElem, [size])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do! Thanks!


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.
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rgolea rgolea Apr 7, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

We can publish a new major version, so users will not get the code until they explicitly opt in to it.

- auto: Adjusts to the maximum existing space inside the `rootElem`
- default: Height adjusts to 400 px on widescreens and 250 px on small screens
Example:
```
var Graph = require('p2p-graph')
//default
var graph = new Graph('.root', {height: 'default', width: 'default'})
//auto
var graph = new Graph('.root', {height: 'auto', width: 'auto'})
//fixed
var graph = new Graph('.root', {height: 200, width: 400})
//function
var graph = new Graph('.root', {height: function () { ... }, width: function () { ... }})
```

### graph.add(peer)

Add a peer to the graph. The `peer` object should contain:
Expand Down
42 changes: 31 additions & 11 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,18 @@ var COLORS = {
}
}

function TorrentGraph (root) {
function TorrentGraph (root, size) {
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')
Copy link
Owner

Choose a reason for hiding this comment

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

Long lines are hard to read. It's usually best to hit enter so they don't get this long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do that. Thank you!

}

function scale () {
var len = model.nodes.length
return len < 10
Expand All @@ -56,8 +61,23 @@ function TorrentGraph (root) {
}
}

var width = root.offsetWidth
var height = (window.innerWidth >= 900) ? 400 : 250
function getSize () {
var height = (window.innerWidth >= 900) ? 400 : 250
var width = root.offsetWidth

if (size) {
if (typeof size.width === 'number') width = size.width
if (typeof size.width === 'function') width = size.width()
if (typeof size.height === 'number') height = size.height
if (typeof size.height === 'function') height = size.height()
if (size.height === 'auto') height = root.offsetHeight
}

return {
width: width,
height: height
}
}

var focus

Expand All @@ -72,12 +92,13 @@ function TorrentGraph (root) {
target.parents.push(link.source)
})

var _size = getSize()
var svg = d3.select(root).append('svg')
.attr('width', width)
.attr('height', height)
.attr('width', _size.width)
.attr('height', _size.height)

var force = d3.layout.force()
.size([width, height])
.size([_size.width, _size.height])
.nodes(model.nodes)
.links(model.links)
.on('tick', function () {
Expand Down Expand Up @@ -239,16 +260,15 @@ function TorrentGraph (root) {
}

function refresh (e) {
width = root.offsetWidth
height = (window.innerWidth >= 900) ? 400 : 250
_size = getSize()

force
.size([width, height])
.size([_size.width, _size.height])
.resume()

svg
.attr('width', root.offsetWidth)
.attr('height', height)
.attr('width', _size.width)
.attr('height', _size.width)
}

function childNodes (d) {
Expand Down
Loading