On 03/16/2016 06:34 PM, Petr Vobornik wrote:
On 03/11/2016 10:09 AM, Pavel Vomacka wrote:


On 03/10/2016 06:08 PM, Petr Vobornik wrote:
On 02/25/2016 03:50 PM, Pavel Vomacka wrote:


On 02/17/2016 06:29 PM, Petr Vobornik wrote:
On 02/15/2016 04:20 PM, Pavel Vomacka wrote:


On 02/12/2016 01:52 PM, Pavel Vomacka wrote:


On 02/11/2016 12:31 PM, Pavel Vomacka wrote:
Hello,

The canvas of the graph had static size. This patch fixes this issue
and from now the graph canvas is resized according to the window
size.

Pavel Vomacka


Because of changes in previous patch I'm sending also this one again.
Plus I fixed some jslint warnings.

And again a link to the ticket:
https://fedorahosted.org/freeipa/ticket/5647 .

--
Pavel^3 Vomacka


And another change in the code. This patch adds checking whether a svg
element even exists. And don't add 'col-sm-12' class to the svg
element
any more. This class just added useless paddings to the element.

--
Pavel^3 Vomacka


Hi,

thanks for the patch.
Hi,

thank you for reviewing.

1. I don't like the fact that the resize handler registered in
initialize method is active forever, even when viewing other facets.
I moved the handler to the topology graph facet. It is also removed
after hide event is emited.
2. The code will probably fail if there is other svg element present
on the page.

$('svg') searches for all svg elements in DOM, such search is usually
slow and undeterministic. It is better to use a stored reference(if
possible) or limit the search to some parent element, e.g. TopoGraph
can store and then use its container.

Would be funny if there were 2 graphs.
Yep, you are right. I avoid using this type of searching in this patch.


3. Why is there the toFixed(1) call? Or more specifically on that
position? It hides the fact that toFixed transforms Number to String
and then '-' operator with Number on the right casts it back to Number.
The toFixed(1) was used just because we don't need so accurate numbers,
but in this patch this function is not used any more.

4. width could be just: this._svg.parent().width()
The width is now solved by using this.content.width() in topology graph facet. I think that the calculating of width and height should be at the
same place. That is why I didn't put calculating of width into the
TopoGraph.

5. Your approach for bottom padding works well but I don't like that
the component assumes that there is some col-sm-12 element on a page
whose right padding is actually equal to space on the left of the svg.
I agree, fixed.

#1 and #5 makes me think that the resize logic should be moved
topology facet. Something like:

* register resize handler on facet's 'show' event
* unregister resize handler on facet's 'hide' event (will solve #1)
* on window resize, compute the size in topology facet, call new
.resize(width, height) method of TopoGraph

Then, we wouldn't have to search whole DOM for 'svg' elements to check
if page is visible. The bottom padding can be obtained by:
parseInt(this.content.css('paddingLeft')) where 'this' is facet.

I followed these tips and here is a new patch.

--
Pavel^3 Vomacka


1.
-    width: 960,
-    height: 500,

Graph even without this patch allows to set initial size in a
constructor, e.g.:

E.g. so he could also use:
  this.graph = new topology_graph.TopoGraph({
     nodes: data.nodes,
     links: data.links,
     suffixes: data.suffixes
     height: height,
     width: width
 });

IMO we should leave some default size there, e.g. the old 960x500 so
that the graph is shown even without explicit configuration.

Ok, I put the default size back, but into graph specification as you
write here.

Ah, I badly expressed myself, sorry. I wanted to leave the original code on its place(TopoGraph). The above was just example what is possible with or without the change because it is not obvious from code.
Default size is returned back now.


2.
-    update: function() {
+    update: function(height, width) {

Update method should not required size params. E.g. if it should
trigger only data update. So it should contain at least a doc string
that the values are optional. Maybe it should be a single param.


These parameters are not required so I add doc string and also changed
them to single param.

Looks good.


--
Pavel^3 Vomacka



>From b222b002af92d5008f1f502f29ac02ac9b296aaa Mon Sep 17 00:00:00 2001
From: Pavel Vomacka <pvoma...@redhat.com>
Date: Thu, 25 Feb 2016 15:02:04 +0100
Subject: [PATCH] Resize topology graph canvas according to window size

The size of svg element is calculated when the topology graph facet is load
and then every time when the window is resized. The resize event listener
is removed after the topology graph facet emits hide event.

https://fedorahosted.org/freeipa/ticket/5647
---
 install/ui/src/freeipa/topology.js       | 48 +++++++++++++++++++++++++++++---
 install/ui/src/freeipa/topology_graph.js | 13 +++++++++
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/install/ui/src/freeipa/topology.js b/install/ui/src/freeipa/topology.js
index 6e67484cc7542765056f0887928a64e4dc576836..c534ec5ca14cbd3f07a58c652117dfefd3a3f816 100644
--- a/install/ui/src/freeipa/topology.js
+++ b/install/ui/src/freeipa/topology.js
@@ -431,14 +431,39 @@ topology.TopologyGraphFacet = declare([Facet, ActionMixin, HeaderMixin], {
     init: function(spec) {
         this.inherited(arguments);
         var graph = this.get_widget('topology-graph');
+        var listener = this.resize_listener.bind(this, graph);
 
         on(this, 'show', lang.hitch(this, function(args) {
-            graph.update();
+            var size = this.calculate_canvas_size();
+            graph.update(size);
+
+            $(window).on('resize', null, listener);
         }));
 
         on(graph, 'link-selected', lang.hitch(this, function(data) {
             this.set_selected_link(data.link);
         }));
+
+        on(this, 'hide', function () {
+            $(window).off('resize', null, listener);
+        });
+    },
+
+    resize_listener: function(graph) {
+        var size = this.calculate_canvas_size();
+
+        graph.resize(size.height, size.width);
+    },
+
+    calculate_canvas_size: function() {
+        var space = parseInt(this.content.css('paddingLeft'), 10);
+        var height = $(window).height() - this.content.offset().top - space;
+        var width = this.content.width();
+
+        return {
+            height: height,
+            width: width
+        };
     },
 
     set_selected_link: function(link) {
@@ -448,7 +473,8 @@ topology.TopologyGraphFacet = declare([Facet, ActionMixin, HeaderMixin], {
 
     refresh: function() {
         var graph = this.get_widget('topology-graph');
-        graph.update();
+        var size = this.calculate_canvas_size();
+        graph.update(size);
     }
 });
 
@@ -802,7 +828,10 @@ topology.TopologyGraphWidget = declare([Stateful, Evented], {
         return deferred.promise;
     },
 
-    update: function() {
+    /**
+     * @param {Object} size - dict contains height and width value. (optional)
+     */
+    update: function(size) {
         this._update_view();
 
         if (IPA.domain_level < topology.required_domain_level) return;
@@ -815,9 +844,14 @@ topology.TopologyGraphWidget = declare([Stateful, Evented], {
                     suffixes: data.suffixes
                 });
                 this._bind_graph_events(this.graph);
+                if (size) {
+                    this.resize(size.height, size.width);
+                }
                 this.graph.initialize(this.visualization_cnt_el);
-
             } else {
+                if (size) {
+                    this.resize(size.height, size.width);
+                }
                 this.graph.update(data.nodes, data.links, data.suffixes);
             }
         }), function(error) {
@@ -897,6 +931,12 @@ topology.TopologyGraphWidget = declare([Stateful, Evented], {
         return this.topology_view_el;
     },
 
+    resize: function(height, width) {
+        if (this.graph) {
+            this.graph.resize(height, width);
+        }
+    },
+
     _init_widgets: function() {
     },
 
diff --git a/install/ui/src/freeipa/topology_graph.js b/install/ui/src/freeipa/topology_graph.js
index 87f53134e24d2c861974ae20b5f8a5f21a9b1d28..1c0a92eeab211d545897f35abcd81882b0d86fa9 100644
--- a/install/ui/src/freeipa/topology_graph.js
+++ b/install/ui/src/freeipa/topology_graph.js
@@ -488,6 +488,19 @@ topology_graph.TopoGraph = declare([Evented], {
             .attr("transform", transform);
     },
 
+    resize: function(height, width) {
+        if (!(isNaN(height) || isNaN(width))) {
+            this.height = height < 0 ? 0 : height;
+            this.width = width < 0 ? 0 : width;
+
+            if (this._svg) {
+                this._svg
+                    .attr('width', this.width)
+                    .attr('height', this.height);
+            }
+        }
+    },
+
     constructor: function(spec) {
         lang.mixin(this, spec);
     }
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to