Bug 87497

Summary: Web Inspector: Tabbed Settings Screen
Product: WebKit Reporter: eustas.bug
Component: Web Inspector (Deprecated)Assignee: eustas.bug
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, caseq, eustas.bug, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Tabbed settings screen, "settings" tab
none
Tabbed settings screen, "shortcuts" tab
none
Patch
none
Patch
yurys: review+
Patch yurys: review+

Description eustas.bug 2012-05-25 06:14:08 PDT
Created attachment 144055 [details]
Tabbed settings screen, "settings" tab

Combine settings screen and shortcuts screen to a single tabbed settings screen.

This will make shortcuts screen more discoverable.
Comment 1 eustas.bug 2012-05-25 06:15:00 PDT
Created attachment 144056 [details]
Tabbed settings screen, "shortcuts" tab
Comment 2 eustas.bug 2012-05-28 01:30:42 PDT
Created attachment 144300 [details]
Patch
Comment 3 Andrey Kosyakov 2012-05-28 06:10:50 PDT
Comment on attachment 144300 [details]
Patch

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

Generally looks good, but see below for a few nits.

> Source/WebCore/inspector/front-end/HelpScreen.js:61
> +    _createCloseButton: function() {

style: { needs to be on a line by its own.

> Source/WebCore/inspector/front-end/SettingsScreen.js:62
> +        function appendSection(name) {

style: { => new line

> Source/WebCore/inspector/front-end/SettingsScreen.js:140
> +    _getTabbedPane: function()

_getOrCreateTabbedPane

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:62
> +        view.element.className = "help-content";
> +
> +        var container = view.element.createChild("div", "help-container");

Can help container be merged into the view element (i.e. help-content)?
Comment 4 eustas.bug 2012-05-28 22:38:20 PDT
Comment on attachment 144300 [details]
Patch

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

>> Source/WebCore/inspector/front-end/HelpScreen.js:61
>> +    _createCloseButton: function() {
> 
> style: { needs to be on a line by its own.

Fixed.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:62
>> +        function appendSection(name) {
> 
> style: { => new line

Fixed.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:140
>> +    _getTabbedPane: function()
> 
> _getOrCreateTabbedPane

Done.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:62
>> +        var container = view.element.createChild("div", "help-container");
> 
> Can help container be merged into the view element (i.e. help-content)?

No problems.
Comment 5 eustas.bug 2012-05-28 22:42:28 PDT
Created attachment 144448 [details]
Patch
Comment 6 Andrey Kosyakov 2012-05-28 23:42:31 PDT
Comment on attachment 144448 [details]
Patch

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

LGTM

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:63
> +        var container = view.element;
> +        container.className = "help-content";
> +        container.addStyleClass("help-container");

Just view.element.className = "help-content help-container".
Comment 7 Yury Semikhatsky 2012-05-29 00:36:29 PDT
Comment on attachment 144448 [details]
Patch

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

> Source/WebCore/inspector/front-end/SettingsScreen.js:639
> +WebInspector.SettingsScreen.SettingsTab = "settings";

WebInspector.SettingsScreen.Tabs = {
  Settings: "Settings",
  Shortcuts: "Shortcuts"
}
like we do with constants in other places.

> Source/WebCore/inspector/front-end/SettingsScreen.js:645
> + * @extends {WebInspector.TabbedPane}

@extends should go right after @constructor, the general rule is to mind alphabetic order of @<name>
Comment 8 eustas.bug 2012-05-29 00:53:05 PDT
Comment on attachment 144448 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SettingsScreen.js:639
>> +WebInspector.SettingsScreen.SettingsTab = "settings";
> 
> WebInspector.SettingsScreen.Tabs = {
>   Settings: "Settings",
>   Shortcuts: "Shortcuts"
> }
> like we do with constants in other places.

Good idea, done.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:645
>> + * @extends {WebInspector.TabbedPane}
> 
> @extends should go right after @constructor, the general rule is to mind alphabetic order of @<name>

Fixed.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:63
>> +        container.addStyleClass("help-container");
> 
> Just view.element.className = "help-content help-container".

Done.
Comment 9 eustas.bug 2012-05-29 00:56:23 PDT
Created attachment 144468 [details]
Patch
Comment 10 Andrey Kosyakov 2012-05-29 03:06:43 PDT
Committed r118747: <http://trac.webkit.org/changeset/118747>