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

[Mads Kiilerich]

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.

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

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

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

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