Bug 222428 – Web Inspector: Refine CSS Grid overlay options

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

[Razvan Caliman]

- Move options to top of list
- Rename section header to "Grid Overlay Options"
- Drop the "Show" prefix from labels
- Reorder and set default values for options:
        * Track sizes: ON
        * Line numbers: ON
        * Labels for line names: OFF
        * Labels for area names: OFF
        * Extended grid lines: OFF

[Radar WebKit Bug Importer]

<rdar://problem/74751569>

[Razvan Caliman]

Created attachment 421689 [details]
Patch

[Devin Rousso]

Comment on attachment 421689 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:70
> +        let settingsGroup = new WI.SettingsGroup(WI.UIString("Grid Overlay Options", "Page Overlay Options @ Layout Panel Section Header", "Heading for list of grid overlay options"));

This should really be named `"Page Overlay Options"` since we're already inside a section named "Grid" and out existing documentation refers to it as the "Page Overlay".

> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:73
> +        settingsGroup.addSetting(WI.settings.gridOverlayShowTrackSizes, WI.UIString("Track Sizes", "Track sizes @ Layers Panel Grid Overlay Setting", "Label for option to toggle the track sizes setting for CSS grid overlays"));

I'm not sure we want to drop the "Show *" in these strings.  I can see a situation where a developer unfamiliar with CSS grid might mistakenly think that these checkboxes are used to control whether WebKit supports this CSS feature, not something changing in the page overlay.  I realize that this is in a "Page Overlay Options" subsection, but I think we should keep the "Show *" so that it's explicitly clear what these boxes are doing.

> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.css:36
> +.details-section.layout-css-grid.collapsed > .content {

Rather than add a new rule, can you `.content:not(.collapsed)` above?

[Razvan Caliman]

Comment on attachment 421689 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSGridSection.js:73
>> +        settingsGroup.addSetting(WI.settings.gridOverlayShowTrackSizes, WI.UIString("Track Sizes", "Track sizes @ Layers Panel Grid Overlay Setting", "Label for option to toggle the track sizes setting for CSS grid overlays"));
> 
> I'm not sure we want to drop the "Show *" in these strings.  I can see a situation where a developer unfamiliar with CSS grid might mistakenly think that these checkboxes are used to control whether WebKit supports this CSS feature, not something changing in the page overlay.  I realize that this is in a "Page Overlay Options" subsection, but I think we should keep the "Show *" so that it's explicitly clear what these boxes are doing.

We discussed this in the team meeting and decided to drop the  "Show" prefix because it is:
1) needlessly repetitive; it applies to all five options. 
2) redundant because the label is associated with a checkbox input which implies an on/off state change.

In the same spirit, if "Page Overlay Options" doesn't require disambiguation because it's nested under the "Grid" section, then it stands to reason that labels under the "Page Overlay Options" subsection relate to the overlay configuration and do not toggle CSS features.

>> Source/WebInspectorUI/UserInterface/Views/LayoutDetailsSidebarPanel.css:36
>> +.details-section.layout-css-grid.collapsed > .content {
> 
> Rather than add a new rule, can you `.content:not(.collapsed)` above?

Yes, that works too.

[Razvan Caliman]

Created attachment 421819 [details]
Patch

[Patrick Angle]

Comment on attachment 421819 [details]
Patch

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

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:182
> +localizedStrings["Area names @ Layers Panel Grid Overlay Setting"] = "Area Names";

This should probably be `... @ Layout Panel Overlay Options` or similar, since the section is now labeled `Page Overlay Options`. Additionally, the `Layers` panel is a different panel entirely.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:848
> +localizedStrings["Line names @ Layers Panel Grid Overlay Setting"] = "Line Names";

Dito :182

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:850
> +localizedStrings["Line numbers @ Layers Panel Grid Overlay Setting"] = "Line Numbers";

Dito :182

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1326
> +localizedStrings["Show extended lines @ Layers Panel Grid Overlay Setting"] = "Extended Grid Lines";

Dito :182

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1550
> +localizedStrings["Track sizes @ Layers Panel Grid Overlay Setting"] = "Track Sizes";

Dito :182

[Razvan Caliman]

Created attachment 422060 [details]
Patch

[Razvan Caliman]

Comment on attachment 421819 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:182
>> +localizedStrings["Area names @ Layers Panel Grid Overlay Setting"] = "Area Names";
> 
> This should probably be `... @ Layout Panel Overlay Options` or similar, since the section is now labeled `Page Overlay Options`. Additionally, the `Layers` panel is a different panel entirely.

Good catch! It was a slip of the keyboard and then copied over and over.

[Blaze Burg]

Comment on attachment 422060 [details]
Patch

r=me

[EWS]

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

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