[Webkit-unassigned] [Bug 85061] Web Inspector: Add "Show keymap" button to settings screen.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85061





--- Comment #6 from eustas.bug at gmail.com  2012-04-28 05:00:40 PST ---
(From update of attachment 139199)
View in context: https://bugs.webkit.org/attachment.cgi?id=139199&action=review

>> Source/WebCore/ChangeLog:6
>> +        Added "Show keymap" button to settings screen + minor LnF adjustments.
> 
> I've had hard time doing the guesswork as to what "LnF" means. Please use the "UI" term instead.

Done.

>> Source/WebCore/ChangeLog:12
>> +        * English.lproj/localizedStrings.js: add "Miscellaneous" and "Show keyboard shortcuts reference"
> 
> The explanation style is "full sentence" ("Something like this.") - see http://trac.webkit.org/changeset/43259 as an example given in http://www.webkit.org/coding/contributing.html#changelogs

OK, thanks.

>> Source/WebCore/ChangeLog:19
>> +        * inspector/front-end/helpScreen.css: LnF adjustmens
> 
> typo: adjustmens -> adjustments

fixed

>> Source/WebCore/ChangeLog:21
>> +        (.help-button): button styles
> 
> styles -> style?

rephrased

>> Source/WebCore/ChangeLog:24
>> +        (.help-content select): ajdust select LnF
> 
> typo: ajdust -> adjust (with 4 lines that follow)

fixed

>> Source/WebCore/inspector/front-end/SettingsScreen.js:109
>> +    }
> 
> I'd rarther pass WebInspector.shortcutsScreen.bind(WebInspector.shortcutsScreen) to createButton.

Done. But still afraid of the race condition.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:159
>> +    _createButton: function(name, listener)
> 
> Please consider adding a jsdoc to the new method.
> 
> optional: Seemingly, "uiName" would be more intuitive, since "name" may refer to the "name" DOM attribute.

Done

>> Source/WebCore/inspector/front-end/SettingsScreen.js:163
>> +        var button = document.createElement("button");
> 
> JFYI, we've got a respective utility method in utilities.js, so you can rewrite this as:
> 
> var button = p.createChild("button", "help-button");
> thereby avoiding the next line and "p.appendChild(button);"

Great. Thanks.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:165
>> +        button.appendChild(document.createTextNode(WebInspector.UIString(name)));
> 
> Drop WebInspector.UIString(), you're already calling it on the call site (which is the preferred approach, btw)

Surely. Fixed.

>>> Source/WebCore/inspector/front-end/SettingsScreen.js:241
>>> +        var div = document.createElement("div");
>> 
>> Why do you need a wrapper around "select"? Can the "help-shadowbox" class be applied directly to the "select" element?
> 
> Why did this change? Is this "minor LnF" change? Please split it into a separate patch.

@Alexander: applying shadow to "select" doesn't work because of "select" element nature.

@Andrey: Splitting to a separate patch will make UI look inconsistent in between these patches.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.