[Freeipa-devel] [PATCH] 0005 webui: topology graph: canvas resizes itself according to the window size

[Petr Vobornik]

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.

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.


-- 
Petr Vobornik