Page MenuHomePhabricator

Allow comment actions to be grouped; group Differential "Review" and "Revision" actions

Authored by epriestley on Dec 29 2016, 9:20 PM.
Referenced Files
F13488493: D17114.diff
Sun, Jul 21, 11:33 AM
F13483098: D17114.id41157.diff
Sat, Jul 20, 6:55 AM
F13473201: D17114.diff
Wed, Jul 17, 12:37 PM
F13455181: D17114.id41171.diff
Sun, Jul 14, 9:11 AM
F13446878: D17114.diff
Sat, Jul 13, 2:27 AM
Unknown Object (File)
Tue, Jul 9, 2:45 PM
Unknown Object (File)
Sun, Jul 7, 11:10 PM
Unknown Object (File)
Sun, Jul 7, 1:08 PM



Ref T11114. Differential has more actions than it once did, and may have further actions in the future.

Make this dropdown a little easier to parse by grouping similar types of actions, like "Accept" and "Reject".

(The action order still needs to be tweaked a bit.)

Test Plan

Screen Shot 2016-12-29 at 1.17.16 PM.png (348×324 px, 38 KB)

Diff Detail

rP Phabricator
Lint Not Applicable
Tests Not Applicable

Event Timeline

one complaint we (FreeBSD) get when we enable abandon for anyone is that the top action is abandon, not accept, and this causes it get used accidentally. Does this change that behavior?

Yes, although the next change will tweak order a little further.

After this change, "review" actions (accept, reject, resign) are always first.

They are always followed by "revision" actions (abandon, close, commandeer, reclaim, plan changes, reopen, request review).

Other actions (change project tags, change subscribers, other third-party stuff) appear at the bottom.

Within the groups, the actions are currently alphabetical by internal class name (or something). The next diff will give them more logical orders, probably:

  • accept, reject, resign
  • plan changes, request review, close, reopen, abandon, reclaim, commandeer

The newer stacked action UI also has an additional line of explanatory text ("This revision will be removed from review queues until it is revised.") which might help catch this. Not sure that's final, but if it sticks around we could make that bold/red/flashy/animated/enormous for rare actions, conceivably, if revising order alone is insufficient:

Screen Shot 2016-12-29 at 1.39.23 PM.png (414×845 px, 38 KB)

I am sorry that frowny cat is a luddite who can't abide the inevitable march of technological progress, but CSS animations are the future of UI.

In a super extreme case, we could do this:

Abandon Revision: [ ] Check this box to confirm that you want to ABANDON this revision. 
                   ^                                                ^
                   |                                                |
                   +- Must be checked to continue.                  |
                                      Flashing, animated red text. -+

However, I suspect that grouping/reordering will resolve this concern without more drastic steps.

However, I suspect that grouping/reordering will resolve this concern without more drastic steps.

Agreed. I suspect that this was mostly the case of seeing two "A" words near each other (and possibly just being used to +A+

chad edited edge metadata.
This revision is now accepted and ready to land.Dec 30 2016, 5:55 PM