[Webkit-unassigned] [Bug 84708] Web Inspector: Shortcuts screen UI polish

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from eustas.bug at gmail.com  2012-04-26 10:18:00 PST ---
(From update of attachment 138989)
View in context: https://bugs.webkit.org/attachment.cgi?id=138989&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You need to explain what has happened and why here (enumerate changes you've done briefly).

Done.

>> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64
>> +    Left: { code: 37, name: "<Left>" },           // also NUM_WEST
> 
> Make sure these are getting localized (get into English.lproj/localizedStrings.js)

These strings are not localized.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:82
>> +        var minMax = totalHeight;
> 
> It is not clear what minMax stands for.

Clarifying comment added.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:83
>> +        var minMaxIdx = -1;
> 
> Please do not use abbreviations (should be Index)

Done

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:86
>> +        for (idx = 0; idx < orderedSections.length; ++idx) {
> 
> for (var i = 0; i < ... ++i) {
> }

Done.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:-80
>> -        // This manual layout ugliness should be gone once WebKit implements
> 
> The underlying issue it not fixed though - what if user resizes the screen?

I'm planning to turn 2 column table to 2 inlined divs in farther patches. So it will be 1 or 2 columned, depending container on width.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:106
>> +            for (idx = stopIndices[columnIndex - 1]; idx < stopIndices[columnIndex]; ++idx) {
> 
> No need for { } around single line blocks.

Done.

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