[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 #3 from Alexander Pavlov (apavlov) <apavlov at chromium.org>  2012-04-27 08:22:57 PST ---
(From update of attachment 139199)
View in context: https://bugs.webkit.org/attachment.cgi?id=139199&action=review

I'm leaving the screenshot analysis to pfeldman, otherwise looks mostly good, with some comments to address.

> 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.

> 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

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

typo: adjustmens -> adjustments

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

styles -> style?

> Source/WebCore/ChangeLog:24
> +        (.help-content select): ajdust select LnF

typo: ajdust -> adjust (with 4 lines that follow)

> 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.

> 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);"

> 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?

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