[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