[PATCH 0 of 7 v3] diff: add collapse to simple diff and make it configurable

[Mads Kiilerich]

On 01/05/2016 08:36 PM, Jan Heylen wrote:
>
>>> --- a/kallithea/templates/changeset/diff_block.html     Sat Jan 02
>>> 11:53:38 2016 +0100
>>> +++ b/kallithea/templates/changeset/diff_block.html     Sat Jan 02
>>> 11:54:11 2016 +0100
>>> @@ -31,6 +31,12 @@
>>>        <span target="${'diff-container-%s' % (id(change))}"
>>> class="diff-collapse-button">↑ ${_('Collapse Diff')} ↑</span>
>>>    </div>
>>>    <div class="diff-container" id="${'diff-container-%s' % (id(change))}">
>>> +    <span style="float:right;margin-bottom:3px;margin-right:30px">
>>> +        <label>
>>> +            ${_('Show inline comments')}
>>> +
>>> ${h.checkbox('',checked="checked",class_="show-inline-comments",id_for="diff-container-%s"
>>> % (id(change)))}
>>> +        </label>
>>> +    </span>
>>>    %for FID,(cs1, cs2, change, path, diff, stats) in change.iteritems():
>>>        <div id="${FID}_target" style="clear:both;margin-top:25px"></div>
>>>        <div id="${FID}" class="diffblock  margined comm">
>>
>> id() will essentially just take the memory address of the variable. That is
>> not good to use here. Other objects might get the same address when this
>> goes out of scope. Actually, it seems like you in this case only need a
>> globally unique identifier?
> Indeed, I thought I get a unique identifier by "id(change)", but I'm
> not sure what to use in replacement?

What should it be unique among? How many of these are there and what do 
they represent?

>> I also see more use case for collapsing the individual file diffs (after
>> they have been reviewed so the reviewer can focus on the rest) than for
>> collapsing everything (leaving almost nothing on the page, only to guide the
>> reader somewhere else).
> I agree, but the current collapse/expand code isn't functional for
> this at this moment, maybe we can change the div box css class
> changeset-header or code-header to be collapseble or expandable, with
> the use of an existing javascript thing. Would it be feasible to
> include jqueryui? We could use https://jqueryui.com/accordion

Yes, we could use more fancy UI. But we should also make sure the UI 
doesn't get more inconsistent. A mix of different effects and styles is 
not pleasant to use. For example, I find it annoying that our file 
navigation is "application in the browser" with fake URLs while the rest 
of Kallithea is page based with quite reasonable URLs and predictable 
navigation.

/Mads