[webkit-reviews] review denied: [Bug 170779] Web Inspector: Modify CSS number values with up key and down key : [Attachment 323845] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Oct 15 15:33:56 PDT 2017
Matt Baker <mattbaker at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 170779: Web Inspector: Modify CSS number values with up key and down key
https://bugs.webkit.org/show_bug.cgi?id=170779
Attachment 323845: Patch
https://bugs.webkit.org/attachment.cgi?id=323845&action=review
--- Comment #7 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 323845
--> https://bugs.webkit.org/attachment.cgi?id=323845
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=323845&action=review
r-, only because this needs a test for the new EditingSupport function.
> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:244
> + } else
I think this would be cleaner as:
if (!modified)
return;
event.preventDefault();
...
Actually, preventDefault gets called on line 250 below, so you should only need
the early return.
> Source/WebInspectorUI/UserInterface/Views/EditingSupport.js:280
> +WI.modifyNumericValue = function(element, modifyValue)
This was existing code, but it would still be good to add a layout test now
that it's included in EditingSupport.
I think it could use a better name too. I didn't really know what `modifyValue`
was, or what this function did to `element` until I got to the new code in
SpreadsheetTextField later. What do you think about:
WI.incrementElementValue = function(element, amount)
{
....
}
I think "numeric" is implied, but it could be `incrementNumericElementValue` if
you think that's more descriptive. Kind of a long name though.
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:236
> + modifyValue /= 10;
Instead of multiplying and dividing, you could do:
let amount;
if (event.metaKey)
amount = 100;
else if (event.shiftKey)
amount = 10;
else if (event.altKey)
amount = 0.1;
else
amount = 1;
if (event.key === "ArrowDown")
amount = -amount;
More information about the webkit-reviews
mailing list