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

[Pavel Vomacka]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pvomacka-0005-6-Resize-topology-graph-canvas-according-to-window-siz.patch
Type: text/x-patch
Size: 4735 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160321/7d9a961a/attachment.bin>