Hide seen transactions by default in all modern applications
ClosedPublic
Actions

Authored by epriestley on Feb 14 2014, 4:32 AM.

Details

Reviewers

btrahan
chad

Maniphest Tasks

T2222: Implement ApplicationTransactions in Differential

Commits

Restricted Diffusion Commit
rP7a96de10f07a: Hide seen transactions by default in all modern applications

Summary

Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions.

This might need some Design help.

Test Plan

The tricky part here is the anchor rule, which deals with the case where you follow a link to T123#4, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest:

{F112891}

Diff Detail

Repository

rP Phabricator

Branch

xacthide

Lint

Lint Passed

Unit

Tests Passed

Event Timeline

epriestley added inline comments.Feb 14 2014, 4:34 AM

src/applications/transactions/view/PhabricatorApplicationTransactionView.php
88	This won't currently treat inline-only comments as comments. It probably should, but I think I'm going to generalize the idea of a "comment-like transaction" soon anyway to share some of the grouping code.

I could imagine this being contentious, though I agree this is the best default.

I personally liked the yellow treatment that we used to have in Differential's. Would be happy to put it back, unless you guys prefer this.

yellow sounds good. i parse that as an "and" thing - this behavior AND the #comment_in_question gets a yellow border

webroot/rsrc/js/application/differential/behavior-show-all-comments.js
47	prolly need to add some class to target to make it yellow

Yeah -- my recollection is that everyone was pretty happy about this when it landed in Differential originally, but we can definitely tweak it or make it app-by-app if there's contention.

Let me do a yellow version.

webroot/rsrc/js/application/differential/behavior-show-all-comments.js
47	Oh, the yellow highlight bit happens with a separate listener (`phabricator-watch-anchor`).

Here are yellow/blue verisons, feel free to shoot me a diff if you want to tweak anything.

{F113138}

{F113139}

Closed by commit rP7a96de10f07a (authored by @epriestley).

Is the default hide behaviour configurable?

No, it's not. Do you not like this behavior?

(I think we might be a little too quick to fold transactions right now, and it might be worthwhile to tweak some of these constants.)

In D8229#13, @epriestley wrote:

No, it's not. Do you not like this behavior?

(I think we might be a little too quick to fold transactions right now, and it might be worthwhile to tweak some of these constants.)

Yes, a little quick for my taste. Also, I find the size of the scrollbar an immediate cue as to how long the thread is. So I always click "Show", anyways.