[webkit-reviews] review granted: [Bug 178286] Web Inspector: [PARITY] Styles Redesign: clicking on the go-to arrow in Computed tab should work : [Attachment 323818] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 15 15:30:58 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 178286: Web Inspector: [PARITY] Styles Redesign: clicking on the go-to
arrow in Computed tab should work
https://bugs.webkit.org/show_bug.cgi?id=178286

Attachment 323818: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 323818
  --> https://bugs.webkit.org/attachment.cgi?id=323818
Patch

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

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.
css:101
> + at keyframes style-property-highlight {
> +    from { background-color: yellow; }
> +}

Shouldn't this have some kind of `to`?

>
Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection
.js:110
> +	   if (!this.didInitialLayout)
> +	       this.updateLayout();

This seems peculiar. You should have a comment describing why we force a layout
here. It seems you could do the same "defer until layout the property to
highlight", but given this is a user action this probably isn't too bad.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:45
> +	   this._property.__view = this;

__view is already used elsewhere. Every `Views/*View.js` sets __view. So I
suggest using a different name to avoid potential conflicts. Maybe
__spreadsheetStyleProperty or __propertyView.


More information about the webkit-reviews mailing list