Bug 74465 - Give a resize handle for sidebyside diffs.
Summary: Give a resize handle for sidebyside diffs.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-13 17:05 PST by Ojan Vafai
Modified: 2011-12-14 10:35 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.03 KB, patch)
2011-12-13 17:06 PST, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2011-12-13 18:31 PST, Ojan Vafai
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-12-13 17:05:03 PST
Give a resize handle for sidebyside diffs.
Comment 1 Ojan Vafai 2011-12-13 17:06:56 PST
Created attachment 119113 [details]
Patch
Comment 2 Ryosuke Niwa 2011-12-13 17:17:30 PST
Comment on attachment 119113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119113&action=review

jQuery provides drag & drop support. Why are we not using it?

> Websites/bugs.webkit.org/code-review.js:67
> +  var minLeftSideWidth = 10;
> +  var maxLeftSideWidth = 90;

I'd rename them to something like minLeftSideRatio to signify the fact these are percent values.

> Websites/bugs.webkit.org/code-review.js:1728
> +  function generateFileDiffResizeStyleElement() {
> +    // FIXME: Once we support calc, we can replace this with something that uses the attribute value.

Really? We can't use CSSOM to generate style rules?

> Websites/bugs.webkit.org/code-review.js:1734
> +        'left: ' + i + '%' +

Can we make this a function? e.g.
cssValueWithPercent(name, value) { return value + ': ' + value + '%;' }

> Websites/bugs.webkit.org/code-review.js:1750
> +  $(document).bind('mousemove', function(e) {

I don't see why we don't want to spell out element.

> Websites/bugs.webkit.org/code-review.js:1751
> +    if (file_diff_being_resized) {

Can we do early exit instead?
Comment 3 Ojan Vafai 2011-12-13 18:31:01 PST
Comment on attachment 119113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119113&action=review

>> Websites/bugs.webkit.org/code-review.js:1728
>> +    // FIXME: Once we support calc, we can replace this with something that uses the attribute value.
> 
> Really? We can't use CSSOM to generate style rules?

We could, but it would be considerably more verbose and less readable without any significant benefits.

>> Websites/bugs.webkit.org/code-review.js:1734
>> +        'left: ' + i + '%' +
> 
> Can we make this a function? e.g.
> cssValueWithPercent(name, value) { return value + ': ' + value + '%;' }

Could. It doesn't really seem worth the added complexity though. As it is, it's very clear what's going on.
Comment 4 Ojan Vafai 2011-12-13 18:31:19 PST
Created attachment 119131 [details]
Patch
Comment 5 Ojan Vafai 2011-12-13 18:33:19 PST
(In reply to comment #2)
> jQuery provides drag & drop support. Why are we not using it?

As best I can tell, It's not built for something like this. It's built for dragging absolutely positioned elements around.
Comment 6 Ojan Vafai 2011-12-14 10:35:48 PST
Committed r102796: <http://trac.webkit.org/changeset/102796>