[webkit-reviews] review granted: [Bug 175343] Web Inspector: Styles Redesign: hook up real data to spreadsheet style editor : [Attachment 319559] 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 175343: Web Inspector: Styles Redesign: hook up real data to spreadsheet
style editor
https://bugs.webkit.org/show_bug.cgi?id=175343

Attachment 319559: Patch

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




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

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

r=me

This looks really nice!  I know I'm in the minority here, but I miss the
selector icons :(

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:33
> +	  
this.element.classList.add(WI.SpreadsheetCSSStyleDeclarationEditor.StyleClassNa
me);
> +	   this._style = style;

Style: add newline

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
js:63
> +	   this.layout();

NIT: should be `needsLayout(WI.View.LayoutReason.Dirty)`

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:38
> +	   this._delegate = delegate || null;
> +
> +	   this._style = style;

Style: unnecessary newline

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:165
> +		   let originString = "";
> +		   switch (this._style.ownerRule.type) {

Style: I'd personally add a newline for readability, but it's up to you

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:201
> +	       this._selectorElement.append(WI.UIString("Style Attribute"));

NIT: this could just be `textContent`

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
92
> +	       let section =
style[WI.SpreadsheetRulesStyleDetailsPanel.RuleSection];
> +
> +	       if (!section) {

Style: unnecessary newline