Code review comment for lp:~abentley/launchpad/display-cleanup

Revision history for this message
Abel Deuring (adeuring) wrote :

(16:40:10) adeuring: abentley: I think "this.current_batch.mustache_model.bugtasks.length + 1" in line 113 is wrong. Shouldn't that be "... -1"?
(16:40:35) adeuring: (line 113 of the diff)
(16:41:09) adeuring: abentley: no, that should be "+0"...
(16:41:13) abentley: adeuring: So, the +1 is trying to compensate for start being 0-indexed.
(16:41:47) adeuring: abentley: right. But for 5 results and start = 0, we want to show "results 1-> 5"
(16:41:56) adeuring: while we get "1 to 6"
(16:42:08) adeuring: abentley: just click on any of the links ;)
(16:42:15) abentley: Hmm, so why does it look right in use?
(16:42:36) adeuring: and compare the number in "n->m of k" with the real number of results ;)
(16:42:49) abentley: adeuring: that's an estimate, though.
(16:43:26) adeuring: abentley: well, k is an estimate (at least for StormRangeFactory), but we known the number of results in the batch precisely
(16:44:07) abentley: adeuring: You're right, I'll fix it.
(16:44:15) adeuring: abentley: cool, thanks
(16:46:07) adeuring: abentley: another quirk: For the first batch, the "first" and "previous" links are gray, but still active. "unlinking" them is something for antother branch, I assume?
(16:46:30) adeuring: (same for the last batch and the last/next links)
(16:47:39) abentley: adeuring: I would prefer if the CSS was fixed so that inactive links don't show as links.
(16:48:33) adeuring: abentley: right, makes sense. So, r=me, and file a bug about the links?
(16:49:02) abentley: adeuring: Thanks. I'll file a bug.

review: Approve

« Back to merge proposal