[webkit-reviews] review granted: [Bug 179625] Web Inspector: ButtonNavigationItem should support image and text button style : [Attachment 326810] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 179625: Web Inspector: ButtonNavigationItem should support image and text
button style
https://bugs.webkit.org/show_bug.cgi?id=179625

Attachment 326810: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:131
> +	   for (let key in WI.ButtonNavigationItem.Style)
> +	      
this.element.classList.remove(WI.ButtonNavigationItem.Style[key]);

You could write this in one line.

   
this.element.classList.remove(...Object.values(WI.ButtonNavigationItem.Style));

> Source/WebInspectorUI/UserInterface/Views/ButtonNavigationItem.js:171
> +		   this._labelElement =
this.element.appendChild(document.createElement("span"));

I realize that `_update()` isn't called that often, but it would be nice if we
didn't have to create a new element each time.	Also, it seems like you don't
even really need to save this to a member variable, so I'd either rename it to
a local variable `labelElement` or add a check to make sure it isn't recreated
with each `_update()`.

> Source/WebInspectorUI/UserInterface/Views/CheckboxNavigationItem.js:73
> +    Checked: "checkbox-navigation-item-checked",

This name implies it will only be fired when the checkbox is checked, not when
it is unchecked.  I'd rename it to "Update" to match the native event name.

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.css:61
> +.navigation-bar .item.checkbox {
> +    padding: 1px 8px 3px;
> +}
> +
> +.navigation-bar .item.checkbox label {
> +    -webkit-padding-start: 3px;
> +}

We should move this to its own CheckboxNavigationItem.css file.  I find it
confusing when I'm looking for the styles of a particular class and there is no
corresponding *.css file, and instead everything is in some parent file.