Page MenuHomePhabricator

Use standard UI elements to render pull requests in Releeph
ClosedPublic

Authored by epriestley on Apr 13 2014, 5:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 21, 11:05 AM
Unknown Object (File)
Thu, Dec 19, 12:53 AM
Unknown Object (File)
Fri, Dec 13, 5:22 AM
Unknown Object (File)
Sat, Dec 7, 7:01 AM
Unknown Object (File)
Sat, Dec 7, 7:01 AM
Unknown Object (File)
Sat, Dec 7, 7:01 AM
Unknown Object (File)
Sat, Dec 7, 5:58 AM
Unknown Object (File)
Tue, Dec 3, 5:17 AM

Details

Reviewers
btrahan
chad
Maniphest Tasks
Restricted Maniphest Task
T3092: Responsively design Releeph
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP35df988036f7: Use standard UI elements to render pull requests in Releeph
Summary

Ref T3718. Ref T3644. Ref T3092. Switches from the Releeph UI elements to standard ones. I'll attach some screenshots.

Also fixes CSRF against the request action endpoint.

Test Plan
  • Viewed request details.
  • Took actions on a request from detail page.
  • Viewed request list.
  • Took actions on a request from list page.
  • Used keyboard shortcuts to navigate list.
  • Used keyboard shortcuts to take actions.
  • Simulated errors.
  • Viewed on devices.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Use standard UI elements to render pull requests in Releeph.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: btrahan, chad.
epriestley added tasks: Restricted Maniphest Task, T3092: Responsively design Releeph, Restricted Maniphest Task.

Here's a summary of the changes:

InterfaceOldNew
Detail Page
Screen_Shot_2014-04-13_at_10.44.00_AM.png (1×1 px, 222 KB)
Screen_Shot_2014-04-13_at_10.44.09_AM.png (1×1 px, 219 KB)
List of Requests
Screen_Shot_2014-04-13_at_10.44.20_AM.png (1×1 px, 233 KB)
Screen_Shot_2014-04-13_at_10.44.28_AM.png (1×1 px, 227 KB)
Focused Request (Keyboard)
Screen_Shot_2014-04-13_at_10.44.37_AM.png (1×1 px, 199 KB)
Screen_Shot_2014-04-13_at_10.44.43_AM.png (1×1 px, 195 KB)
Device View
Screen_Shot_2014-04-13_at_10.48.46_AM.png (744×396 px, 143 KB)
Screen_Shot_2014-04-13_at_10.48.30_AM.png (744×396 px, 138 KB)
Status Error
Screen_Shot_2014-04-13_at_10.50.38_AM.png (1×1 px, 208 KB)
Screen_Shot_2014-04-13_at_10.51.25_AM.png (1×1 px, 205 KB)

I'd like to avoid another insurrection, so please give us feedback on these UI changes if you think they're significantly disruptive to your workflows.

(The new element is slightly larger overall than the old one was, mostly because the message is rendered using a large font size. Tentatively, I eventually intend to let each request include more than one commit (i.e., so users can request an entire branch, like on GitHub) and show the commits in a table with the messages summarized at the bottom of this UI. However, this may not happen for a while.)

epriestley added a subscriber: Unknown Object (MLST).Apr 13 2014, 6:03 PM

(For good measure.)

Robinpuri retitled this revision from Use standard UI elements to render pull requests in Releeph to fuck off.Apr 13 2014, 7:29 PM
Robinpuri retitled this revision from fuck off to :).
Robinpuri changed the visibility from "Public (No Login Required)" to "All Users".
Robinpuri changed the edit policy from "All Users" to "Robinpuri (Robin puri)".
epriestley retitled this revision from :) to Use standard UI elements to render pull requests in Releeph.Apr 13 2014, 7:30 PM
epriestley changed the visibility from "All Users" to "Public (No Login Required)".
epriestley changed the edit policy from "Robinpuri (Robin puri)" to "All Users".

The majesty of open source.

We have AphrontMultiColumnView for 2-up display, if you want. It is device-ready.

I think we should drop the multi-column layout:

Currently, pull requests in Releeph always request one commit, and the UI roughly puts information about the requestor in one column and information about the commit in the other column. This is reasonable in SVN and some Git workflows, but makes less sense in branch-based Git workflows, and isn't what users coming from GitHub will expect (on GitHub, pull requests are essentially always for "all commits on a branch", not "specific commit X"). I think that sticking with this model will reduce the usefulness of the tool in general and create a lot of user confusion. Assuming we move away from this model, this UI won't make sense anyway.

Beyond that, no other objects use multiple columns, and none really have a compelling argument for it. Since nothing else does this, none of the infrastructure has support for it. For example, we have a drag-and-drop control for reordering fields in configuration, but it doesn't support choosing which column they will appear on.

Overall, supporting multiple columns requires a large amount of custom code (this diff removes almost a thousand lines in net, and more big chunks of code can be dropped beyond that) for what I think is a pretty questionable gain. At least for me, these new UIs and supporting interactions look much better and are easier to read.

Of course, they're also different...

epriestley edited edge metadata.
  • Remove some more dead code (organizing custom fields into columns).
btrahan edited edge metadata.
This revision is now accepted and ready to land.Apr 14 2014, 6:45 PM

Dead sexy @epriestley. I don't believe there will be any pitchforks over this.

Good news, my ad has been approved:

Screen_Shot_2014-04-14_at_2.50.13_PM.png (179×268 px, 12 KB)

Saw the ad, redesign looks great.

We got 112 clicks from Facebook employees and no complaints, so I'm going to move this forward. Thanks for taking a look!


For future readers: if and when there are complaints down the line, we're happy to make adjustments to this interface. We can be most effective in addressing specific issues (like: field X used to do Y, but doesn't any more; I'm colorblind and Z is harder to read than it used to be; field Q isn't very important, but now has a lot more space dedicated to it; etc etc).

We probably can't be very effective in addressing "put it back like it was and never change anything" feedback, because this kind of feedback conflicts directly with all other goals, and we made every reasonable effort we could to collect this kind of "general dissatisfaction" feedback here, before making this change.

epriestley updated this revision to Diff 20904.

Closed by commit rP35df988036f7 (authored by @epriestley).