Bug 52226 - allow sidebysideifying individual files in the code review tool
Summary: allow sidebysideifying individual files in the code review tool
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-11 11:11 PST by Ojan Vafai
Modified: 2011-01-11 15:16 PST (History)
3 users (show)

See Also:


Attachments
Patch (2.84 KB, patch)
2011-01-11 11:12 PST, Ojan Vafai
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
screenshot (123.07 KB, image/png)
2011-01-11 11:12 PST, Ojan Vafai
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-01-11 11:11:25 PST
allow sidebysideifying individual files in the code review tool
Comment 1 Ojan Vafai 2011-01-11 11:12:07 PST
Created attachment 78557 [details]
Patch
Comment 2 Ojan Vafai 2011-01-11 11:12:31 PST
Created attachment 78558 [details]
screenshot
Comment 3 Adam Barth 2011-01-11 12:06:17 PST
The fonts don't seem to match the existing side-by-side links.  Also, maybe we should have one link that changes text based on the current state?  I'm worried about overloading the reviewtool with lots of UI.
Comment 4 Ojan Vafai 2011-01-11 12:31:40 PST
(In reply to comment #3)
> The fonts don't seem to match the existing side-by-side links. 

It's just inheriting the font. I'll fix that.

> Also, maybe we should have one link that changes text based on the current state?  I'm worried about overloading the reviewtool with lots of UI.

I'm sympathetic to this. I think this is valuable though. We can certainly remove it later if people find it to be too much. It's just a few lines of code.

I don't have plans to add any other UI after this. The only tasks I have planned for the review tool:
-Make side-by-side diffs more minimal by actually putting removed lines to the left of added lines.
-Store whether you want side-by-side diffs in the cookie so you can always load the one you prefer by default.
Comment 5 Adam Barth 2011-01-11 14:18:20 PST
Can we hide the link that's not meaningful (e.g., side-by-side when we're already in side-by-side mode)?  Also, I would vertically center the link in the H1.
Comment 6 Adam Barth 2011-01-11 14:52:36 PST
Comment on attachment 78557 [details]
Patch

Lets land this with the cosmetic changes discussed above.  It would be nice to add the link-hiding in a followup patch.
Comment 7 Ojan Vafai 2011-01-11 15:16:41 PST
Committed r75559: <http://trac.webkit.org/changeset/75559>