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

[Thomas De Schampheleire]

On Jan 5, 2016 8:36 PM, "Jan Heylen" <heyleke at gmail.com> wrote:
>
> On Tue, Jan 5, 2016 at 5:12 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> > On 01/02/2016 01:15 PM, Jan Heylen wrote:
> >>
> >> Hi,
> >>
> >> third version of this patchset, with some extra fixes and partly
> >> refactoring of the applicable code (diff_block)
> >> more work to get both templates uniform are WIP.
> >
> >
> > Thanks. Much better!
> I'm on a learning curve here :-)
>
> >
> > As you can see, I took some of your changes and reworked them to my
liking
> > instead of just commenting. I hope that is ok. Some of the more
> > "controversial" changes have been left out, but they should be easier
to do
> > now and be easier to present cleanly.
> >
> > General comments:
> > Please try to
> > * put one change in each changeset
> > * make good commit messages that explain why the change is made and
what is
> > achieved
> > * order them by "risk" and "likely to be taking without more
discussion" -
> > refactorings first, then features
> I tried to split into small changesets, but I'll try to do even better
> splitting, I tried to give enough info in the commit, but I'll try to
> give even more info, I ordered them in order of "taking in risk", but
> I apparently have a different judgement ;-)
>
> >
> >> --- 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?
>
> >
> > I don't know how much more convincing the commit messages can be made,
but
> > I'm very reluctant to introduce code for hiding the diff after fetching
it.
> > I think it must be lazily loaded when the diff actually is shown.
> I agree, but as said, that is not the goal of this changeset. And
> indeed, changing the default collapse behaviour upstream is not my
> goal.
>
> >
> > 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
>

JQueryUi  seems overkill to me since collapsing file diffs could be
accomplished very easily with standard js/jquery, right? Attach a click
handler to all collapse links and use 'this' (possibly one of its parents)
to get hold of the diff that needs to be collapsed.

/Thomas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sfconservancy.org/pipermail/kallithea-general/attachments/20160106/72a5a775/attachment-0001.html>