Differential - finesse Differential diff view controller
ClosedPublic
Actions

Authored by btrahan on Feb 18 2015, 11:15 PM.

Details

Reviewers

epriestley

Maniphest Tasks

T7229: Create comments in diff views is not possible

Commits

Restricted Diffusion Commit
rP1d72a5f6832d: Differential - finesse Differential diff view controller

Summary

Fixes T7229. Some usability issues around this controller - basically you can't leave comments with it and its not particular useful compared to the revision page.

Ergo, if there is a revision associated with a given diff, just re-direct back to the revision page with the proper diff loaded.

Test Plan

Tried to view a diff on the standalone controller attached to a revision and instead was re-directed to the revision view page with the proper diff loaded.

Diff Detail

Repository

rP Phabricator

Branch

T7229

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/differential/controller/DifferentialDiffViewController.php:34	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 4573
Build 4587: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

btrahan updated this revision to Diff 28471.Feb 18 2015, 11:15 PM

btrahan added a task: T7229: Create comments in diff views is not possible.

btrahan retitled this revision from to Differential - finesse Differential diff view controller.

btrahan updated this object.

btrahan edited the test plan for this revision. (Show Details)

btrahan added a reviewer: epriestley.

Herald added subscribers: epriestley, Korvin. · View Herald TranscriptFeb 18 2015, 11:15 PM

Some inlines, take 'em or leave 'em.

src/applications/differential/controller/DifferentialDiffViewController.php
28–39	Let's just redirect to `/Dxxx?id=yyy`? I don't think this page is actually ever useful for diffs that are attached to revisions.
122	...then we can get rid of this.
src/applications/differential/view/DifferentialTransactionView.php
148–153	I'm not sure this is actually useful, since the text directly adjacent to it is already linked. Specifically, I don't understand what a reasonable user could be thinking that this would help them with. I also think this is potentially more confusing, because this jumps you to a specific place within the diff, despite being labeled "On Diff X", which I would expect to take me to the base URI of that diff, especially when I have two different links to click which are adjacent with different labels. But I don't think we should send the user there because that navigation action seems pointless. Do you think this is really an improvement? e.g., do you find this more clear? I don't really care strongly, it just seems kind of weird/unexpected to me.