Bug 221556 – Web Inspector: Add settings UI for CSS Grid overlay

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Razvan Caliman]

Depends on https://bugs.webkit.org/show_bug.cgi?id=221145

Add UI to the Layout panel to show grid overlay configuration options.

[Razvan Caliman]

Created attachment 419595 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/74100005>

[Razvan Caliman]

Created attachment 419601 [details]
[Image] With patch applied

[Razvan Caliman]

(In reply to Razvan Caliman from comment #1)
> Created attachment 419595 [details]
> Patch

EWS doesn't deal well with patches not based on master. There's a dependency to Bug 	221145. Once that lands, the current patch applies cleanly on top and EWS bots should be happy.

[Devin Rousso]

Comment on attachment 419595 [details]
Patch

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

this looks generally fine to me, but I kinda feel like we should have these settings be in a gear `optionsElement` for the containing `WI.DetailsSection` instead (especially since they're global settings and not per overlay) so that they don't use/waste all the vertical space (the screenshot (attachment 419601 [details]) has lots of empty space next to the checkboxes that feels very wasteful)

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:715
> +localizedStrings["Grid overlay settings heading"] = "Grid overlay settings";
> +localizedStrings["Grid overlay setting: Show line numbers"] = "Show line numbers";
> +localizedStrings["Grid overlay setting: Show line names"] = "Show line names";
> +localizedStrings["Grid overlay setting: Show extended lines"] = "Show extended lines";
> +localizedStrings["Grid overlay setting: Show area names"] = "Show area names";
> +localizedStrings["Grid overlay setting: Show track sizes"] = "Show track sizes";

NIT: we usually put the string to be shown in the UI before an "@" (e.g. "Show line numbers @ Grid Overlay Settings") or have the description/location be in parenthesis (e.g. "Show line numbers (Grid Overlay Settings)")

Also, I think these are all missing comments based on the `WI.UIString` calls with these key.  Rather than edit this file manually, you can just run `./Tools/Scripts/extract-localizable-js-strings --utf8 Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js Source/WebInspectorUI/UserInterface' and have it all done for you :)

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:45
> +        for (let setting of gridOverlaySettings) {

Style: no `{` `}` for single-line control flow

> Source/WebInspectorUI/UserInterface/Controllers/OverlayManager.js:46
> +            setting.addEventListener(WI.Setting.Event.Changed, this._handleGridSettingChanged, this);

If you think about it, by having this loop you're actually using _more_ lines of code than if you'd just written it out manually.  I'd suggest just doing it manually so that it's explicitly clear in each case that an event listener is being added (e.g. when doing global search usually only a few lines of before/after context are shown).

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.css:32
> +.css-grid-section .heading,
> +.css-grid-section .title {

```
    .css-grid-section :is(.heading, .title)
```

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:41
> +                WI.UIString("Show line numbers", "Grid overlay setting: Show line numbers", "Label for option to toggle the line numbers setting for CSS grid overlays")

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:45
> +                WI.UIString("Show line names", "Grid overlay setting: Show line names", "Label for option to toggle the line names setting for CSS grid overlays")

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:49
> +                WI.UIString("Show extended lines", "Grid overlay setting: Show extended lines", "Label for option to toggle the extended lines setting for CSS grid overlays")

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:53
> +                WI.UIString("Show area names", "Grid overlay setting: Show area names", "Label for option to toggle the area names setting for CSS grid overlays")

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:57
> +                WI.UIString("Show track sizes", "Grid overlay setting: Show track sizes", "Label for option to toggle the track sizes setting for CSS grid overlays")

Style: missing trailing `,`

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:58
> +            ]

Style: missing trailing `,`

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

lol `WI.SettingsGroup` (and `WI.SettingEditor`) were really only created for the Settings Tab, but I suppose no reason they can't be used elsewhere too?

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:90
> +        for (let [setting, label] of this._gridOverlaySettingsMap) {

Rather than generate a `Map` in the `constructor`, can't we just use it here instead?
```
    settingsGroup.addSetting(WI.settings.gridOverlayShowLineNumbers, 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."));
    ...
```

[Blaze Burg]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 419595 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419595&action=review
> 
> this looks generally fine to me, but I kinda feel like we should have these
> settings be in a gear `optionsElement` for the containing
> `WI.DetailsSection` instead (especially since they're global settings and
> not per overlay) so that they don't use/waste all the vertical space (the
> screenshot (attachment 419601 [details]) has lots of empty space next to the
> checkboxes that feels very wasteful)

The current design explicitly emulates other browser tools. We can consider having the grid options collapse to a gear, but let's revisit that when everything is put together.

[Razvan Caliman]

Created attachment 419858 [details]
Patch

[Blaze Burg]

Comment on attachment 419858 [details]
Patch

Looks like it needs to be rebased again.

[Razvan Caliman]

Created attachment 419894 [details]
Patch

[Devin Rousso]

Comment on attachment 419894 [details]
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

[Razvan Caliman]

Created attachment 419972 [details]
Patch

Carry over R+ from Comment #10. Address code review notes

[EWS]

Committed r272737: <https://commits.webkit.org/r272737>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419972 [details].