573671 - TBPL could ellide the entries for long pushes

Status: RESOLVED DUPLICATE of bug 731261

(Tree Management Graveyard :: TBPL, defect)

Description

[Daniel Brooks [:db48x]]

Attachment: untested, but it looks right

A push with a large number of commits soon becomes annoyingly long on the TBPL page. We should keep the number of visible commits to a reasonable number, with something the user can click to show the rest.

Comment 1

[Daniel Brooks [:db48x]]

Attachment: still untested. this changes the [expand] text to [collapse] and back when it's clicked on

Comment 2

[Markus Stange [:mstange]]

Thanks Daniel, I'll look at it tomorrow. Unfortunately we've missed the Weave landing now :-)

Comment 3

[(no longer active)]

There is still the IndexedDB landing to look forward to, IINM.  ;-)

Comment 4

[Daniel Brooks [:db48x]]

err, there is at least one small problem with that patch :)

Comment 5

[Daniel Brooks [:db48x]]

I set up http://db48x.net/tbpl/ as a test and found a few things to fix. "↓ 1919 hidden changesets [Expand]" is certainly nice to see :)

alas, clicking on the expander doesn't do anything. probably something I'm too tired to see; I'll look again in the morning.

Comment 6

[Daniel Brooks [:db48x]]

Attachment: this one's tested and had the kinks worked out

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 453756 [details] [diff] [review]
this one's tested and had the kinks worked out

>+      var patches = [];
>+      for each (p in push.patches)
>+        patches.push(p);

var patches = push.patches.slice();

>+      var nhidden = patches.length - 10;

Please put the 10 into Config.defaultVisibleNumberOfPatchesPerPush or something like that and increase it to 20.

>+      var hidden = (nhidden > 0) && patches.splice(10, nhidden);
>+      var experid = 'expander-'+ push.toprev;
>+      var expeeid = 'expandee-'+ push.toprev;
>+      var collapserid = 'collapser-'+ push.toprev;
>+      hidden && expanders.push([experid, expeeid, collapserid].map(function (id) { return "#"+ id; }));

I think putting this into an if (nhidden > 0) block would be clearer than the && magic.

>+      (hidden ? ('\n<span id="'+ experid +'" class="expander">â '+ nhidden +' hidden changesets <a href="#">[Expand]</a></span>\n'+
>+      '<div id="'+ expeeid +'" style="display: none;">\n'+
>+      hidden.map(buildHTMLForPushPatches).join("\n") +
>+      '\n<span id="'+ collapserid +'" class="expander">â '+ nhidden +' collapsable changesets <a href="#">[Collapse]</a></span>\n'+
>+      '</div>\n') : '')+

Please construct this string outside the concatenation chain. It's a little hard to see where the (hidden ? ... : ...) construct ends.
Putting <span>s and <div>s as direct children of a <ul> is unclean. Please wrap them in a <li>.
s/collapsable/collapsible/

>+    for each (e in expanders)

expanders.forEach(function (e) { ... }) please. Don't use for...in on arrays.

>+      var expander = $(e[0]);
>+      var expandee = $(e[1]);
>+      var collapser = $(e[2]);
>+      expander.bind("click", function () { expander.hide(); expandee.slideDown("fast"); });
>+      collapser.bind("click", function () { expandee.slideUp("fast", function () { expander.show(); }); });

This causes flickering. It's especially noticeable at the beginning of the collapse animation. I think that could be fixed by putting the collapser outside the expandee and showing and hiding it at the same time as hiding and showing the expander.

Comment 8

[Daniel Brooks [:db48x]]

(In reply to comment #7)
> >+    for each (e in expanders)
> 
> expanders.forEach(function (e) { ... }) please. Don't use for...in on arrays.

Ooh, nice. Always did want to use modern javascript features on a webpage. :)