[PATCH 1 of 1] diff: diff_block_simple: make it collapsable and default behaviour selectable

[Jan Heylen]

On Thu, Dec 24, 2015 at 5:55 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 12/07/2015 07:20 PM, Jan Heylen wrote:
>>
>> # HG changeset patch
>> # User Jan Heylen <jan.heylen at alcatel-lucent.com>
>> # Date 1448811822 -3600
>> #      Sun Nov 29 16:43:42 2015 +0100
>> # Node ID eecdf2feaa98d065981b7b25f99e6843409f4f75
>> # Parent  543b35dff5e737398ae6eaac0ca98c857c5655fc
>> diff: diff_block_simple: make it collapsable and default behaviour
>> selectable
>
>
> I don't see exactly how this should work and can't test it so I can't
> comment much.
Huh?

>
> We use something similar -
> https://bitbucket.org/Unity-Technologies/kallithea/commits/89ce15150e3e8c1b3acd0eeeb4cad0c97ab8f686
> . How do you think they compare?

I don't know, this is the way it works in diff_block, the one used in
collapsing commit diffs.

>
> I can see how it could be a good idea to not show the big diff ... but I
> think it would be essential that it also didn't send the diff to the client
> before it was shown. Perhaps a bit like the PR creation page loads the list
> of changesets dynamically. (I think it would be even better if it somehow
> could show the diffstat but expand it to the full diff.)

Yes, that would be good, but this is not how this code works today, I
only made sure the simple diff version, used in PR, also supports the
collapsing thing, it is not my intention to change the behavior of the
collapsing feature.

>
>> diff -r 543b35dff5e7 -r eecdf2feaa98
>> kallithea/templates/changeset/diff_block.html
>> --- a/kallithea/templates/changeset/diff_block.html     Thu Dec 03
>> 08:59:26 2015 +0100
>> +++ b/kallithea/templates/changeset/diff_block.html     Sun Nov 29
>> 16:43:42 2015 +0100
>> @@ -65,8 +65,19 @@
>>   </div>
>>   </%def>
>>   -<%def name="diff_block_simple(change)">
>> -
>> +<%def name="diff_block_simple(change, hidden=False)">
>> +<%
>> +if hidden:
>> +    expand_collapse_msg = "↓ ${_'Expand Diff')} ↓"
>> +    container_class = "hidden"
>> +else:
>> +    expand_collapse_msg = "↑ ${_'Collapse Diff')} ↑"
>> +    container_class = ""
>> +%>
>> +<div class="diff-collapse">
>> +  <span target="${'diff-container-%s' % (id(change))}"
>> class="diff-collapse-button">${expand_collapse_msg}</span>
>
>
> expand_collapse_msg gets extra quoting - it doesn't work as it is.
>
>> +</div>
>> +<div class="diff-container ${container_class}" id="${'diff-container-%s'
>> % (id(change))}
>
>
> This is missing an ending '>' ... so it doesn't work and is untested?

I did tested it, I cleaned/refactored the last part just before
publishing so probably that's were it's gone missing, I'll update to
V2, sorry for the confusion.

>
>>     %for op,filenode_path,diff in change:
>>       <div id="${h.FID('',filenode_path)}_target"
>> style="clear:both;margin-top:25px"></div>
>>       <div id="${h.FID('',filenode_path)}" class="diffblock  margined
>> comm">
>> @@ -124,6 +135,7 @@
>>           </div>
>>       </div>
>>     %endfor
>> +</div>
>>   </%def>
>>     <%def name="diff_block_js()">
>> _______________________________________________
>> kallithea-general mailing list
>> kallithea-general at sfconservancy.org
>> http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
>
>