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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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.