Page MenuHomePhabricator

Basic stacked action support for EditEngine
ClosedPublic

Authored by epriestley on Dec 3 2015, 7:38 PM.

Details

Summary

Ref T9132. This still has a lot of rough edges but the basics seem to work OK.

Test Plan

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley updated this revision to Diff 35448.Dec 3 2015, 7:38 PM
epriestley retitled this revision from to Basic stacked action support for EditEngine.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
chad edited edge metadata.Dec 3 2015, 7:41 PM
_______ < omg > ------- \ ^__^ \ (oo)\_______ (__)\ )\/\ ||----w | || ||
chad added a comment.Dec 3 2015, 7:42 PM
_____________________ < this is amooozing > --------------------- \ ^__^ \ (oo)\_______ (__)\ )\/\ ||----w | || ||
chad accepted this revision.Dec 3 2015, 7:52 PM
chad edited edge metadata.
This revision is now accepted and ready to land.Dec 3 2015, 7:52 PM
ftdysa awarded a token.Dec 3 2015, 7:54 PM
chad added a comment.Dec 3 2015, 7:56 PM

I played with this locally and nothing broke!

This definitely needs some more work (drafts don't save the actions, for example, and I have no idea what it does on mobile but probably not anything great, and there are more than a few very rough patches in this code) but it at least mostly works, and I don't think it regresses or breaks anything -- it may just not work as well as it should in some cases.

I think this is the last major piece we need before I can at least try bringing this to Maniphest. Not sure how well that'll go, but hopefully not too bad.

(One other possible issue is "new similar task", not sure if I want to bring that into ApplicationEditor or not yet. I may just sort of hack around it for v0, although having "new similar object" in every application doesn't necessarily seem terrible to me, just also not clearly useful/desirable in many apps.)

webroot/rsrc/js/application/transactions/behavior-comment-actions.js
77–82

aww yeahhh

chad added a comment.Dec 3 2015, 8:04 PM

Yeah, maybe for mobile... or everywhere... just update the select to be "Remove Projects", then we don't need all that code for adding icons?

Two possible issues with that:

  • That might get hairy if we stick with the direct-status-actions in Differential ("Remove Accept Revision" next to "Request Changes"?). Not totally sure what that will look like yet.
  • This interface will also be used in the EditEngine version of "Batch Editor", but it will have a "Remove Projects" action there, so we'd end up with "Remove 'Remove Projects' Action'" or similar.

I think we can get it working OK on mobile in the relatively short term through brute force and then refine this later once we have a clearer idea about other use cases.

The "use a dropdown to pick actions" UI feels a little odd to me, but I think I'll get used to it. It does seem a bit cleaner than the sort of Herald approach (where you select each action from a separate dropdown) in this situation (although I like the Herald approach for Herald).

This revision was automatically updated to reflect the committed changes.