<p dir="ltr"><br>
On Jan 5, 2016 8:36 PM, "Jan Heylen" <<a href="mailto:heyleke@gmail.com">heyleke@gmail.com</a>> wrote:<br>
><br>
> On Tue, Jan 5, 2016 at 5:12 PM, Mads Kiilerich <<a href="mailto:mads@kiilerich.com">mads@kiilerich.com</a>> wrote:<br>
> > On 01/02/2016 01:15 PM, Jan Heylen wrote:<br>
> >><br>
> >> Hi,<br>
> >><br>
> >> third version of this patchset, with some extra fixes and partly<br>
> >> refactoring of the applicable code (diff_block)<br>
> >> more work to get both templates uniform are WIP.<br>
> ><br>
> ><br>
> > Thanks. Much better!<br>
> I'm on a learning curve here :-)<br>
><br>
> ><br>
> > As you can see, I took some of your changes and reworked them to my liking<br>
> > instead of just commenting. I hope that is ok. Some of the more<br>
> > "controversial" changes have been left out, but they should be easier to do<br>
> > now and be easier to present cleanly.<br>
> ><br>
> > General comments:<br>
> > Please try to<br>
> > * put one change in each changeset<br>
> > * make good commit messages that explain why the change is made and what is<br>
> > achieved<br>
> > * order them by "risk" and "likely to be taking without more discussion" -<br>
> > refactorings first, then features<br>
> I tried to split into small changesets, but I'll try to do even better<br>
> splitting, I tried to give enough info in the commit, but I'll try to<br>
> give even more info, I ordered them in order of "taking in risk", but<br>
> I apparently have a different judgement ;-)<br>
><br>
> ><br>
> >> --- a/kallithea/templates/changeset/diff_block.html Sat Jan 02<br>
> >> 11:53:38 2016 +0100<br>
> >> +++ b/kallithea/templates/changeset/diff_block.html Sat Jan 02<br>
> >> 11:54:11 2016 +0100<br>
> >> @@ -31,6 +31,12 @@<br>
> >> <span target="${'diff-container-%s' % (id(change))}"<br>
> >> class="diff-collapse-button">↑ ${_('Collapse Diff')} ↑</span><br>
> >> </div><br>
> >> <div class="diff-container" id="${'diff-container-%s' % (id(change))}"><br>
> >> + <span style="float:right;margin-bottom:3px;margin-right:30px"><br>
> >> + <label><br>
> >> + ${_('Show inline comments')}<br>
> >> +<br>
> >> ${h.checkbox('',checked="checked",class_="show-inline-comments",id_for="diff-container-%s"<br>
> >> % (id(change)))}<br>
> >> + </label><br>
> >> + </span><br>
> >> %for FID,(cs1, cs2, change, path, diff, stats) in change.iteritems():<br>
> >> <div id="${FID}_target" style="clear:both;margin-top:25px"></div><br>
> >> <div id="${FID}" class="diffblock margined comm"><br>
> ><br>
> ><br>
> > id() will essentially just take the memory address of the variable. That is<br>
> > not good to use here. Other objects might get the same address when this<br>
> > goes out of scope. Actually, it seems like you in this case only need a<br>
> > globally unique identifier?<br>
> Indeed, I thought I get a unique identifier by "id(change)", but I'm<br>
> not sure what to use in replacement?<br>
><br>
> ><br>
> > I don't know how much more convincing the commit messages can be made, but<br>
> > I'm very reluctant to introduce code for hiding the diff after fetching it.<br>
> > I think it must be lazily loaded when the diff actually is shown.<br>
> I agree, but as said, that is not the goal of this changeset. And<br>
> indeed, changing the default collapse behaviour upstream is not my<br>
> goal.<br>
><br>
> ><br>
> > I also see more use case for collapsing the individual file diffs (after<br>
> > they have been reviewed so the reviewer can focus on the rest) than for<br>
> > collapsing everything (leaving almost nothing on the page, only to guide the<br>
> > reader somewhere else).<br>
> I agree, but the current collapse/expand code isn't functional for<br>
> this at this moment, maybe we can change the div box css class<br>
> changeset-header or code-header to be collapseble or expandable, with<br>
> the use of an existing javascript thing. Would it be feasible to<br>
> include jqueryui? We could use <a href="https://jqueryui.com/accordion">https://jqueryui.com/accordion</a><br>
></p>
<p dir="ltr">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.</p>
<p dir="ltr">/Thomas</p>