RESOLVED FIXED 176033
Web Inspector: Styles Redesign: display "Inherited From" section headers
https://bugs.webkit.org/show_bug.cgi?id=176033
Summary Web Inspector: Styles Redesign: display "Inherited From" section headers
Nikita Vasilyev
Reported 2017-08-28 13:11:05 PDT
In the current styles sidebar UI, we show them as: Media: all [rules inside of @media all {...}] Inherited From: .article [.article rule itself] Bug 175343 misses "Inherited From" and media query section headers.
Attachments
[Image] WIP (168.89 KB, image/png)
2017-09-02 20:48 PDT, Nikita Vasilyev
no flags
Patch (5.17 KB, patch)
2017-09-05 16:52 PDT, Nikita Vasilyev
hi: review+
[Screenshot] With patch applied (427.67 KB, image/png)
2017-09-05 16:57 PDT, Nikita Vasilyev
no flags
Patch (4.81 KB, patch)
2017-09-05 19:47 PDT, Nikita Vasilyev
no flags
Radar WebKit Bug Importer
Comment 1 2017-08-28 13:11:23 PDT
Nikita Vasilyev
Comment 2 2017-09-02 19:35:36 PDT
To manually test this: 1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/inherited.html 2. Inspect h3#title Expected: In the styles sidebar, you should see sections in the following order: h3 {...} h3 🔒 {...} Inherited From: div.in .in {...} div {...} Inherited From: body.out .out {...}
Nikita Vasilyev
Comment 3 2017-09-02 20:48:50 PDT
Created attachment 319757 [details] [Image] WIP
Nikita Vasilyev
Comment 4 2017-09-05 16:52:25 PDT
Nikita Vasilyev
Comment 5 2017-09-05 16:57:21 PDT
Created attachment 319960 [details] [Screenshot] With patch applied
Devin Rousso
Comment 6 2017-09-05 18:15:11 PDT
Comment on attachment 319957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319957&action=review r=me > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:53 > + static createInheritedHeader(style) Is there a reason for this to be public static? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:108 > + let inheritedHeader = WI.SpreadsheetRulesStyleDetailsPanel.createInheritedHeader(style); > + this.element.append(inheritedHeader); I don't think this needs to be in it's own variable, especially if you drop the static: this.element.appendChild(this._createInheritedHeader(style)); or even better, you could just do the append inside the function and name it something like `_appendInheritedHeader` :)
Nikita Vasilyev
Comment 7 2017-09-05 19:46:49 PDT
Comment on attachment 319957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319957&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:53 >> + static createInheritedHeader(style) > > Is there a reason for this to be public static? No reason besides that it can be static since it's a pure function (has no side effects and return value is only determined by its input values). >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:108 >> + this.element.append(inheritedHeader); > > I don't think this needs to be in it's own variable, especially if you drop the static: > > this.element.appendChild(this._createInheritedHeader(style)); > > or even better, you could just do the append inside the function and name it something like `_appendInheritedHeader` :) I like this.element.appendChild(this._createInheritedHeader(style)); Although _appendInheritedHeader would be a bit more brief, I find it more readable when all the append/addSubview operations are in this loop's body.
Nikita Vasilyev
Comment 8 2017-09-05 19:47:27 PDT
WebKit Commit Bot
Comment 9 2017-09-05 20:55:31 PDT
Comment on attachment 319970 [details] Patch Clearing flags on attachment: 319970 Committed r221662: <http://trac.webkit.org/changeset/221662>
WebKit Commit Bot
Comment 10 2017-09-05 20:55:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.