Created attachment 360403 [details] [Video] Demo Currently, Changes panel doesn't have any resource links. Group CSS rules by their resource. Create resource sections with their headers being source links.
<rdar://problem/47617785>
Created attachment 360411 [details] Patch
Created attachment 360412 [details] [Image] With patch applied
Comment on attachment 360411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360411&action=review > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:39 > + padding-top: var(--css-declaration-vertical-padding); We should style the empty message like the one in the Canvas tab > Traces sidebar panel. The message should be centered vertically and horizontally, and use title case to be consistent with similar panels in native apps and Web Inspector. I know this is somewhat unrelated, but what about changing the text to just "No CSS Changes"? > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:68 > +.changes-panel .unchanged.property { Writing the selectors like this sorts better visually: .changes-panel .property.unchanged { ... } .changes-panel .property.added { ... } .changes-panel .property.removed { ... } But I think the way you have them reads better (verb-object). Does anyone else have an opinion? I'll leave it up to you. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:74 > + background-color: hsl(70, 90%, 86%); What prompted the background-color tweaks for .added and .removed? > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:104 > + background-color: hsl(5, 40%, 28%); Same question as above. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:85 > + let rulesList = stylesheets.get(cssRule.ownerStyleSheet); First the map value is referred to as `rulesList`, then later when iterating it is `cssRules`. Let's pick one or the other. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:114 > + _renderRule(cssRule) Since this creates and returns an element, how about `_createRuleElement(cssRule)`?
Comment on attachment 360411 [details] Patch I noticed that adding a new rule, and changing an existing rule, causes two .resource-sections to be added for the same file. Is this intentional?
Comment on attachment 360411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360411&action=review > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:33 > + padding-top: var(--css-declaration-vertical-padding); NIT: I typically put `padding` (a "positional" property) above `font` (a "content" property). > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:44 > + border-bottom: 0.5px solid var(--text-color-quaternary); How does this look in @1x? > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:47 > +.changes-panel .resource-section > .header { NIT: I'd reorder this as follows: position: -webkit-sticky; top: 0; z-index: var(--z-index-header); padding: 4px 8px; background-color: var(--panel-background-color); border-bottom: 0.5px solid var(--text-color-quaternary); white-space: nowrap; overflow: hidden; text-overflow: ellipsis; > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:51 > + border-bottom: 0.5px solid var(--text-color-quaternary); Ditto (>44). >> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:68 >> +.changes-panel .unchanged.property { > > Writing the selectors like this sorts better visually: > > .changes-panel .property.unchanged { > ... > } > > .changes-panel .property.added { > ... > } > > .changes-panel .property.removed { > ... > } > > But I think the way you have them reads better (verb-object). Does anyone else have an opinion? I'll leave it up to you. Personally, I like having commonality when reading (e.g. the common bits are first, so just by looking at the end we can see what's different). > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:83 > + content: "-"; NIT: I typically put `content` below positional properties (e.g. `position`, `margin`, `padding`, etc) but above content properties (e.g. `background`, `border`, `pointer-events`, etc). > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:90 > + content: "+"; Ditto (>83). > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:83 > + let stylesheets = new Map(); This name implies an array, not a map. How about `rulesForStylesheet`? > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:93 > + stylesheets.forEach((cssRules, styleSheet) => { Style: we prefer `for...of` vs `forEach`. for (let [styleSheet, cssRules] of stylesheets) > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:94 > + let resourceSection = document.createElement("section"); For cases like these, I'd rather you put it all on one line for brevity. let resourceSection = this.element.append(document.createElement("section")); > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:97 > + let resourceHeader = document.createElement("div"); Ditto (>94). let resourceHeader = resourceSection.append(document.createElement("div")); > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:99 > + resourceHeader.append(this._createLocationLink(styleSheet)); Style: add a newline before this line. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:104 > + let ruleElement = this._renderRule(cssRule); NIT: you can just inline this. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:122 > + ruleElement.append(selectorElement); Ditto (>94). let selectorElement = ruleElement.appendChild(document.createElement("span")); > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:124 > + let appendProperty = (cssProperty, className) => { NIT: there's no reason to have this be an arrow function, since you don't use this. Let's avoid the "cost" of capturing `this`. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:128 > + stylePropertyView.element.insertAdjacentText("afterbegin", WI.indentString()); > + stylePropertyView.element.insertAdjacentText("beforeend", "\n"); I'm not a fan of adding DOM to "encapsulated" UI objects, as it effectively requires editors of `WI.SpreadsheetStyleProperty` to have to know to never call `this.element.removeChildren` (as an example). Is there a way you can wrap the entire `WI.SpreadsheetStyleProperty` in a container element (and apply `className` to the container as well) and put these before/after within the container? > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:158 > + let options = { NIT: this should be `const`. > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:166 > + let sourceCodeLocation = styleSheet.createSourceCodeLocation(0, 0); NIT: these values should be `const`. const lineNumber = 0; const columnNumber = 0; let sourceCodeLocation = styleSheet.createSourceCodeLocation(lineNumber, columnNumber);
Comment on attachment 360411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360411&action=review >> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:47 >> +.changes-panel .resource-section > .header { > > NIT: I'd reorder this as follows: > > position: -webkit-sticky; > top: 0; > z-index: var(--z-index-header); > padding: 4px 8px; > background-color: var(--panel-background-color); > border-bottom: 0.5px solid var(--text-color-quaternary); > white-space: nowrap; > overflow: hidden; > text-overflow: ellipsis; Could you document your preferred order style on https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#CSS ? Once we agree on the order, I'd like to write a script to sort properties.
Comment on attachment 360411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360411&action=review >>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:47 >>> +.changes-panel .resource-section > .header { >> >> NIT: I'd reorder this as follows: >> >> position: -webkit-sticky; >> top: 0; >> z-index: var(--z-index-header); >> padding: 4px 8px; >> background-color: var(--panel-background-color); >> border-bottom: 0.5px solid var(--text-color-quaternary); >> white-space: nowrap; >> overflow: hidden; >> text-overflow: ellipsis; > > Could you document your preferred order style on https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#CSS ? > > Once we agree on the order, I'd like to write a script to sort properties. It's more of a "general feeling" and less of a hard-set order. Regardless, I'll write up a short list of common properties (obviously I won't list all 500-something CSS properties 😛).
Created attachment 360533 [details] [Image] section borders @1x (In reply to Devin Rousso from comment #6) > > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:44 > > + border-bottom: 0.5px solid var(--text-color-quaternary); > > How does this look in @1x? It looks good, I think. WebKit rounds 0.5px to 1px (started doing it 3 years ago).
Comment on attachment 360411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360411&action=review >> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:74 >> + background-color: hsl(70, 90%, 86%); > > What prompted the background-color tweaks for .added and .removed? I very slightly decreased the contrast after making the lines span the entire width of the panel. >> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:83 >> + content: "-"; > > NIT: I typically put `content` below positional properties (e.g. `position`, `margin`, `padding`, etc) but above content properties (e.g. `background`, `border`, `pointer-events`, etc). It seems to me that `content` is the most important property, when present. >> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:128 >> + stylePropertyView.element.insertAdjacentText("beforeend", "\n"); > > I'm not a fan of adding DOM to "encapsulated" UI objects, as it effectively requires editors of `WI.SpreadsheetStyleProperty` to have to know to never call `this.element.removeChildren` (as an example). Is there a way you can wrap the entire `WI.SpreadsheetStyleProperty` in a container element (and apply `className` to the container as well) and put these before/after within the container? Sounds reasonable.
Created attachment 360616 [details] Patch
Comment on attachment 360616 [details] Patch r=me, nice work!
Comment on attachment 360411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360411&action=review >>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:83 >>> + content: "-"; >> >> NIT: I typically put `content` below positional properties (e.g. `position`, `margin`, `padding`, etc) but above content properties (e.g. `background`, `border`, `pointer-events`, etc). > > It seems to me that `content` is the most important property, when present. `content` only really has an "important" effect for ::before/::after. And even then, if we set a `content: "foo";` but had `display: none;`, having set a `content` property is effectively irrelevant as we aren't showing anything.
Comment on attachment 360616 [details] Patch Clearing flags on attachment: 360616 Committed r240741: <https://trac.webkit.org/changeset/240741>
All reviewed patches have been landed. Closing bug.