[webkit-reviews] review granted: [Bug 178795] Web Inspector: Styles Redesign: typing colon in property name should advance to value field : [Attachment 326953] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 178795: Web Inspector: Styles Redesign: typing colon in property name
should advance to value field
https://bugs.webkit.org/show_bug.cgi?id=178795

Attachment 326953: Patch

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




--- Comment #3 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 326953
  --> https://bugs.webkit.org/attachment.cgi?id=326953
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:29
> +	   (WI.SpreadsheetTextField):

NIT: this should be at the beginning of the WI.SpreadsheetTextField list.  This
happens to me every once in a while for some reason ¯\_(ツ)_/¯

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:177
> +	       this._nameElement.addEventListener("beforeinput", (event) => {

Since you are adding the event listener to the element itself, I'd reorder
these.

    this._nameElement.tabIndex = 0;
    this._nameElement.addEventListener("beforeinput",
this._handleNameBeforeinput.bind(this));

    this._nameTextField = new WI.SpreadsheetTextField(this, this._nameElement,
this._nameCompletionDataProvider.bind(this));

I'd also suggest moving the callback to be a member function.  I typically try
to only use inline event listeners for extremely small (effectively one line)
callbacks, or contextmenu stuff.