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

[Petr Vobornik]

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.

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


-- 
Petr Vobornik