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

[Pavel Vomacka]

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.

> 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.

--
Pavel^3 Vomacka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0005-5-Resize-topology-graph-canvas-according-to-window-siz.patch
Type: text/x-patch
Size: 5191 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160311/378ce926/attachment.bin>