Created attachment 139194 [details] Screenshot: before patch Keyboard shortcuts screen can be opened only with a key shortcut. So users has no chance to know it even exists.
Created attachment 139195 [details] Screenshot: after patch
Created attachment 139199 [details] Patch
Comment on attachment 139199 [details] Patch 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?
Comment on attachment 139199 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=139199&action=review I'm not fan of the UI approach and "Miscellaneous" section in particular. How about moving it to the title bar, perhaps as a link or some iconic button? > Source/WebCore/inspector/front-end/SettingsScreen.js:109 > + function showKeyMap() > + { > + WebInspector.shortcutsScreen.show(); > + } I'd rarther pass WebInspector.shortcutsScreen.bind(WebInspector.shortcutsScreen) to createButton. > 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) >> 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.
Created attachment 139357 [details] Patch
Comment on attachment 139199 [details] Patch 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.
I'd rather convert this pane into a tabbed pane.
Comment on attachment 139357 [details] Patch r- per Pavel's comments.
Keyboard shortcuts are accessible through tab in settings screen.