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

reduce horizontal gap between nodes in topology views #1693

Merged
merged 1 commit into from
Jul 20, 2016

Conversation

rade
Copy link
Member

@rade rade commented Jul 17, 2016

Fixes #1688

@rade
Copy link
Member Author

rade commented Jul 17, 2016

I've tried this on a few topologies, and did searches on them. See, e.g. #1688 (comment). All seems to look fine.

btw, I don't pretend to know what I am doing here. And the fact that I had to adjust labelWidth to prevent overlapping makes me uneasy - I would have expected that to be adjusted automatically with the node separation factor.

@davkal
Copy link
Contributor

davkal commented Jul 20, 2016

It all comes down to the search results being displayed. Sadly, browser support for controlling their layout (foreignObject in SVG) is limited. So you end up with blocks that are wider than they need to be, and assume full width. At that point they are too close together with the neighbour nodes, see lower row here:

screen shot 2016-07-20 at 09 22 46

For the row separation, the more-matches text is blocked:

screen shot 2016-07-20 at 09 26 21

Here is the report I'm using: https://gist.github.com/davkal/b02d9e6c90b73fd39e7d9fbaec11815e

@rade
Copy link
Member Author

rade commented Jul 20, 2016

At that point they are too close together with the neighbour nodes

I had put a screen shot just like that in #1688. I don't see it as a problem. Yes, more separation would be better, but increasing horizontal node space to do that is worse.

For the row separation, the more-matches text is blocked:

That's about vertical spacing, surely. How does that have anything to do with this change?

@rade
Copy link
Member Author

rade commented Jul 20, 2016

That's about vertical spacing, surely. How does that have anything to do with this change?

And the answer is: nothing. I get the same (and more) overlap on our dev admin scope running 'master'.

@davkal
Copy link
Contributor

davkal commented Jul 20, 2016

You're right. I thought rank separation was changed here too, but it wasnt.
The node separation LGTM.

@foot
Copy link
Contributor

foot commented Jul 20, 2016

This LGTM, works nicely on the wide layouts.

@rade rade merged commit d19c0bf into master Jul 20, 2016
@rade rade deleted the 1688-reduce-horizontal-gaps branch July 5, 2017 13:08
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.

3 participants