On Jan 5, 2016 8:36 PM, "Jan Heylen" <[email protected]> wrote: > > On Tue, Jan 5, 2016 at 5:12 PM, Mads Kiilerich <[email protected]> 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
_______________________________________________ kallithea-general mailing list [email protected] http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
