[webkit-reviews] review granted: [Bug 221556] Web Inspector: Add settings UI for CSS Grid overlay : [Attachment 419894] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Razvan Caliman
<rcaliman at apple.com>'s request for review:
Bug 221556: Web Inspector: Add settings UI for CSS Grid overlay
https://bugs.webkit.org/show_bug.cgi?id=221556

Attachment 419894: Patch

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




--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 419894
  --> https://bugs.webkit.org/attachment.cgi?id=419894
Patch

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

r=me with some minor changes

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:71
> +	   let settingsGroup = new WI.SettingsGroup(WI.UIString("Grid overlay
settings", "Grid overlay settings heading", "Heading for list of grid overlay
settings"));

This should probably change to `WI.UIString("Page Overlay Settings", "Page
Overlay Settings @ Layout Panel Section Header", "Heading for list of grid
overlay settings")` to match the "Page Overlay" group above it.

Also, as a general piece of feedback, we usually capitalize headings/titles
when it's not a fully formed sentence.

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:74
> +	   settingsGroup.addSetting(

Style: We don't do this kind of multi-line function call.  If you feel that the
line would be too long, I'd suggest doing something like this
```
    const showLineNumbersLabel = WI.UIString("Show line numbers", "Show line
numbers @ Layers Panel Grid Overlay Setting", "Label for option to toggle the
line numbers setting for CSS grid overlays");
    settingsGroup.addSetting(WI.settings.gridOverlayShowLineNumbers,
showLineNumbersLabel);
```

ditto below