Bug 222428 - Web Inspector: Refine CSS Grid overlay options
Summary: Web Inspector: Refine CSS Grid overlay options
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Razvan Caliman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-25 11:00 PST by Razvan Caliman
Modified: 2021-03-03 15:00 PST (History)
6 users (show)

See Also:


Attachments
Patch (12.05 KB, patch)
2021-02-26 12:11 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (12.10 KB, patch)
2021-03-01 07:07 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch (12.05 KB, patch)
2021-03-03 03:57 PST, Razvan Caliman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Razvan Caliman 2021-02-25 11:00:12 PST
- 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
Comment 1 Radar WebKit Bug Importer 2021-02-25 11:00:57 PST
<rdar://problem/74751569>
Comment 2 Razvan Caliman 2021-02-26 12:11:50 PST
Created attachment 421689 [details]
Patch
Comment 3 Devin Rousso 2021-02-26 12:27:57 PST
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?
Comment 4 Razvan Caliman 2021-03-01 07:05:53 PST
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.
Comment 5 Razvan Caliman 2021-03-01 07:07:55 PST
Created attachment 421819 [details]
Patch
Comment 6 Patrick Angle 2021-03-01 09:47:18 PST
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
Comment 7 Razvan Caliman 2021-03-03 03:57:00 PST
Created attachment 422060 [details]
Patch
Comment 8 Razvan Caliman 2021-03-03 03:58:51 PST
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.
Comment 9 BJ Burg 2021-03-03 14:52:31 PST
Comment on attachment 422060 [details]
Patch

r=me
Comment 10 EWS 2021-03-03 15:00:43 PST
Committed r273847: <https://commits.webkit.org/r273847>

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