[webkit-reviews] review denied: [Bug 178022] Web Inspector: [PARITY] Styles Redesign: clicking on the white space after the property should create a blank property : [Attachment 325230] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Oct 28 23:05:04 PDT 2017


Matt Baker <mattbaker at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 178022: Web Inspector: [PARITY] Styles Redesign: clicking on the white
space after the property should create a blank property
https://bugs.webkit.org/show_bug.cgi?id=178022

Attachment 325230: Patch

https://bugs.webkit.org/attachment.cgi?id=325230&action=review




--- Comment #7 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 325230
  --> https://bugs.webkit.org/attachment.cgi?id=325230
Patch

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

r-, for the following issue:

Style Attribute [--- region 1 ---]
{  [--- region 2 ---]
    [✓] border: none;
}

Clicking in region 2 adds a new property after "border", instead of before.
Clicking in region 1 has the expected behavior. Also, I noticed with this patch
that the open brace is now on its own line. Was this a recent change to our
design, or was it unintentional? Personally I prefer having the open brace on
the same line as the selector. Having it on its own line wastes space, IMO.

Also, clicking in the space before and after the checkbox creates a new
property after "border". My expectation was that the new property would be
added before the "border" property. Chrome expands the hit region for the
checkbox to include the surrounding whitespace, and we may want to do the same.
This is an extreme edge case, so I don't think its necessary to address in this
patch, unless its a simple change.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:125
> +	   let forceRemove = true;

const

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:345
> +	       // Newly added properties are initially blank. It should be
possible to remove them.

I don't think this comment is necessary. The call to _updateStyleText which
sets forceRemove to true has an explanatory comment that suffices.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:348
> +		   let columnDelta = 0;

Both of these should be const.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:202
> +	   let newlyAdded = this._valueTextField.startEditingValue === "";

Since `startEditingValue` is only accessed to test for the newly added property
case, SpreadsheetTextField should just expose a boolean property that performs
this test internally:

get isNewProperty()
{
    return this._startEditingValue === "";
}

`propertyWasAdded` could work as a name too.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:61
> +    get startEditingValue() { return this._startEditingValue; }

See my comment above regarding this property. The name of the backing member
variable is a little confusing to me as well. I had to read the code to figure
it out. Maybe `_valueBeforeEditing` would be better.


More information about the webkit-reviews mailing list