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

[Pavel Vomacka]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0005-4-Resize-topology-graph-canvas-according-to-window-siz.patch
Type: text/x-patch
Size: 4776 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160225/12f004d0/attachment.bin>