Skip to content
This repository was archived by the owner on Jun 27, 2020. It is now read-only.

Added netjsongraph.js visualization to admin site#36

Merged
nemesifier merged 1 commit intoopenwisp:masterfrom
rohithasrk:visualizer-admin
Jun 22, 2017
Merged

Added netjsongraph.js visualization to admin site#36
nemesifier merged 1 commit intoopenwisp:masterfrom
rohithasrk:visualizer-admin

Conversation

@rohithasrk
Copy link
Contributor

This PR aims to add a feature which allows to visualize network topology in admin site. @nemesisdesign Please review.

@coveralls
Copy link

coveralls commented Jun 20, 2017

Coverage Status

Coverage decreased (-0.9%) to 99.121% when pulling c85672d on rohithasrk:visualizer-admin into fefbce5 on netjson:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great progress!

CSS / style issues

  • labels are not visible (black foreground on black background)
  • when the overlay is open there's an unnecessary scrollbar, you should hide it by putting overflow:hidden on the body element but you should remove this CSS once the overlay is closed
  • spacing from page broders look wrong, check CSS margins of the element, but do it after you solve the previous scrollbar issue
  • as agreed on IRC, move the node/link metadata panel to the bottom, but only in the admin overlay

Test

As you know, visualize_view is not executed during tests, so fix that when you can.

$('#content-main form').trigger('submit');
}
else {
var message = 'Error while generating preview';
Copy link
Member

Choose a reason for hiding this comment

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

this is not a preview :-P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. My bad :P

if (e.altKey && e.which == 80) {
// unfocus any active input before proceeding
$(document.activeElement).trigger('blur');
// wait for JSON editor to update the
Copy link
Member

Choose a reason for hiding this comment

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

remove comments mentioning JSON editor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Will do it

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 99.121% when pulling c73de79 on rohithasrk:visualizer-admin into fefbce5 on netjson:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.9%) to 99.121% when pulling c73de79 on rohithasrk:visualizer-admin into fefbce5 on netjson:master.

@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.297% when pulling fb958be on rohithasrk:visualizer-admin into fefbce5 on netjson:master.

@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage decreased (-0.7%) to 99.297% when pulling f898322 on rohithasrk:visualizer-admin into fefbce5 on netjson:master.

@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 3e49480 on rohithasrk:visualizer-admin into 3f29fdb on netjson:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@rohithasrk almost there, more details need to be taken care of, see my inline comments.

Also rebase on master.

inner = overlay.find('.inner'),
visualize_url = $('.visualizelink').attr('data-url');

var openPreview = function() {
Copy link
Member

Choose a reason for hiding this comment

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

rename to openOverlay

$(document).keydown(disableArrowKeys);
};

var closePreview = function () {
Copy link
Member

Choose a reason for hiding this comment

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

rename to closeOverlay

<p><span class="link up">&nbsp;</span>link up</p>
<p><span class="link down">&nbsp;</span>link down</p>
</div>
{% block script %}
Copy link
Member

Choose a reason for hiding this comment

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

we are duplicating information here, try to convert this part in a fragment which can be shared with the frontend template (and included with {% include '../netjsongraph/netjsongraph-script.html' %}) and overridden in accordance with the overriding templates instructions in the README

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been fixed.


def test_topology_visualize_button(self):
t = Topology.objects.first()
t.save()
Copy link
Member

Choose a reason for hiding this comment

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

is this line necessary? Try to remove it


def test_topology_visualize_view(self):
t = Topology.objects.first()
t.save()
Copy link
Member

Choose a reason for hiding this comment

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

idem

data-url="{{ visualize_url }}"
class="visualizelink"
value="{% trans "Visualize Topology" %}"
title="{% trans "Visualize Topology" %} (ALT+P)">
Copy link
Member

Choose a reason for hiding this comment

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

idem

.djnjg-overlay .close:focus,
.djnjg-overlay .close:active{
position: absolute;
right: 3%;
Copy link
Member

Choose a reason for hiding this comment

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

use 10px

.djnjg-overlay .close:active{
position: absolute;
right: 3%;
top: 1.5%;
Copy link
Member

Choose a reason for hiding this comment

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

use 10px

z-index: 11;
width: 100%;
height: 100%;
background: rgba(0, 0, 0, 0.91);
Copy link
Member

Choose a reason for hiding this comment

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

use rgba(0, 0, 0, 0.95);, it's less distracting in this case

css = {'all': [static('netjsongraph/admin.css')]}
js = [static('netjsongraph/receive-url.js'),
static('netjsongraph/strategy-switcher.js')]
css = {'all': [static('netjsongraph/css/style.css'),
Copy link
Member

Choose a reason for hiding this comment

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

netjsongraph/css/style.css must be moved after netjsongraph/css/src/netjsongraph.css because it's an override

@coveralls
Copy link

coveralls commented Jun 21, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a0c4cc7 on rohithasrk:visualizer-admin into 3f29fdb on netjson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a0c4cc7 on rohithasrk:visualizer-admin into 3f29fdb on netjson:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a0c4cc7 on rohithasrk:visualizer-admin into 3f29fdb on netjson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a0c4cc7 on rohithasrk:visualizer-admin into 3f29fdb on netjson:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a0c4cc7 on rohithasrk:visualizer-admin into 3f29fdb on netjson:master.

@nemesifier nemesifier changed the title Added visualizer to admin site Added netjsongraph.js visualization to admin site Jun 22, 2017
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

👍 great work.

@nemesifier nemesifier merged commit f58f87a into openwisp:master Jun 22, 2017
@rohithasrk rohithasrk deleted the visualizer-admin branch June 22, 2017 09:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants