[webkit-reviews] review granted: [Bug 218126] Web Inspector: Promote experimental "Show independent Styles sidebar" setting to "Elements" settings tab and enable by default : [Attachment 417214] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 218126: Web Inspector: Promote experimental "Show independent Styles
sidebar" setting to "Elements" settings tab and enable by default
https://bugs.webkit.org/show_bug.cgi?id=218126

Attachment 417214: Patch v1.0

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Setting.js:197
> +    enableIndependentStylesPanel: new WI.Setting("independent-styles-panel",
true),

FYI if you wanted to be very "accurate" then you'd probably want to replace the
`true` with `WI.Setting.migrateValue("experimental-independent-styles-panel")
?? true`, as that would preserve the old value and remove it from
`localStorage`.

Not necessary to do since this was experimental, but something to keep in mind
as you modify any saved data :)

NIT: while you're at it, can you add "Elements" and "Details" in there
somewhere so it's a bit more clear what it's referring to?  e.g.
"enableElementsTabIndependentStylesDetailsSidebarPanel" :P

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:288
> +	   elementsSettingsView.addSetting(WI.UIString("Styles Sidebar:",
"Styles Sidebar: @ Settings Elements Tab", "Category label for Styles sidebar
settings."), WI.settings.enableIndependentStylesPanel, WI.UIString("Show
independent Styles sidebar", "Show independent Styles sidebar @ Settings
Elements Tab", "Settings tab checkbox label for whether the independent styles
sidebar should be shown"));

It seems a bit repetitive to have "Styles Sidebar" in both the label and the
checkbox text.	How about having the label be something more generic like
"Details Sidebars" so that we could maybe use it for other things in the future
too (e.g. if we instead wanted to allow the Computed panel to be independent
then we could make these into radio buttons)?

Also, we refer to these as "panes" in our documentation
<https://webkit.org/web-inspector/web-inspector-settings/#elements> so please
use that instead of "tab" :)