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

Split the internet node for incoming vs outgoing connections. #566

Merged
merged 1 commit into from
Mar 1, 2016

Conversation

tomwilkie
Copy link
Contributor

Fixes #360, includes #570

Before this change:

screen shot 2015-10-16 at 17 19 56

After this change:

screen shot 2015-10-16 at 17 23 30

@tomwilkie tomwilkie force-pushed the 360-internet-node branch 2 times, most recently from 06ff0dd to 4a041a2 Compare October 17, 2015 16:07
@tomwilkie tomwilkie modified the milestone: 0.9.0 Oct 18, 2015
@peterbourgon
Copy link
Contributor

👎

  • Two internet nodes in the same graph is immediately confusing
  • The heuristic of incoming vs. outgoing connections is nonobvious, nondeterministic, and confuses the codebase
  • The "fix" to the rendered topology is superficial; I believe we can get better results by continuing to iterate on the UI

@tomwilkie tomwilkie force-pushed the 360-internet-node branch 2 times, most recently from 67404df to 2370799 Compare November 10, 2015 13:51
@pidster
Copy link
Contributor

pidster commented Nov 10, 2015

On the basis that the outbound node will be split into labelled services (somehow) and that the two internet nodes are now visibly separate; the two nodes primary label should be "Inbound" and "Outbound" (connections) - not "The Internet" (inbound/outbound).

So reversing the title/subtitle labels in effect.

@tomwilkie
Copy link
Contributor Author

Screen shot showing outgoing internet lower than other nodes (David's work):
screen shot 2015-11-10 at 18 03 07

@tomwilkie
Copy link
Contributor Author

Renamed to "Inbound" / "Outbound" Requests:

screen shot 2015-11-10 at 18 03 07

As this has been an contentious PR, I'd like to get an LGTM from @pidster for the concept, and a second LGTM from someone (@paul?) for the code.

@paulbellamy
Copy link
Contributor

The code looks fine. Conceptually I'm far from convinced this is a good idea.

  • Inbound/Outbound is a complete guess. Right now we have no way of actually telling this.
  • the outbound node will be split into labelled services (somehow) I'd like to see a proof-of-concept that this is can be done usefully before we make this the goal.

@pidster
Copy link
Contributor

pidster commented Jan 12, 2016

LGTM

Sorry for the delay.
I think we need to make progress and then iterate based on feedback.

@tomwilkie
Copy link
Contributor Author

I've rebased this up to date, but the pinning of the internet nodes doesn't seem to be working properly @davkal:
screen shot 2016-02-29 at 15 14 46

@pidster pidster added the dogfood Important for the developer's own use of the project label Feb 29, 2016
@davkal
Copy link
Contributor

davkal commented Mar 1, 2016

  • added JS tests, now for the real world...
  • outbound node positioning seems to work
  • @tomwilkie got a report with incoming nodes to test?
  • still thinking about positioning relative to unconnected node field (which can be big, pushing the out node far below)

@davkal
Copy link
Contributor

davkal commented Mar 1, 2016

This is the unconnected field I'm talking about:
screen shot 2016-03-01 at 13 10 27
Angry torvalds suddenly appeared (initially unconnected, so it was laid out in the field). Let's see if I do the internet positioning step afterwards.

@davkal
Copy link
Contributor

davkal commented Mar 1, 2016

Seems to work:
screen shot 2016-03-01 at 13 12 55

Click on Force Re-layout balances layout nicely, too.

@davkal
Copy link
Contributor

davkal commented Mar 1, 2016

Inbound node positioning seems to shift nodes, but leave edges untouched. Fixing that...

@tomwilkie
Copy link
Contributor Author

@davkal inbound from the internet to the scope container (will need to show them)

report.json.zip

@davkal
Copy link
Contributor

davkal commented Mar 1, 2016

screen shot 2016-03-01 at 14 28 56

@tomwilkie
Copy link
Contributor Author

thanks @davkal! I don't know about you, but I don't think the pinning looks that good - this is without the pinning:
screen shot 2016-03-01 at 13 42 56

@pidster
Copy link
Contributor

pidster commented Mar 1, 2016

I am +1 for splitting The Internet into Outgoing & Incoming per the above.
See also: #1082

I understand that improving the rendering algo that pins the two nodes to the top & bottom respectively, to make a less odd looking diagram will require more work - so let's park that for now.

LGTM without pinning, for now.

@tomwilkie tomwilkie force-pushed the 360-internet-node branch from 54f8e4c to bc48a48 Compare March 1, 2016 14:12
tomwilkie added a commit that referenced this pull request Mar 1, 2016
Split the internet node for incoming vs outgoing connections.
@tomwilkie tomwilkie merged commit 1e3625e into master Mar 1, 2016
@tomwilkie tomwilkie deleted the 360-internet-node branch March 1, 2016 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dogfood Important for the developer's own use of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants