Bug 52226

Summary:

allow sidebysideifying individual files in the code review tool

Product:

WebKit

Reporter:

Ojan Vafai <ojan>

Component:

New Bugs

Assignee:

Ojan Vafai <ojan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric, mihaip

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	abarth: review+, abarth: commit-queue-
screenshot	none

Ojan Vafai

Reported 2011-01-11 11:11:25 PST

allow sidebysideifying individual files in the code review tool

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
View All Add attachment proposed patch, testcase, etc.

Ojan Vafai

Comment 1 2011-01-11 11:12:07 PST

Created attachment 78557 [details] Patch

Ojan Vafai

Comment 2 2011-01-11 11:12:31 PST

Created attachment 78558 [details] screenshot

Adam Barth

Comment 3 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.

Ojan Vafai

Comment 4 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.

Adam Barth

Comment 5 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.

Adam Barth

Comment 6 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.

Ojan Vafai

Comment 7 2011-01-11 15:16:41 PST

Committed r75559: <http://trac.webkit.org/changeset/75559>

Note You need to log in before you can comment on or make changes to this bug.