Bug 84708 - Web Inspector: Shortcuts screen UI polish
Summary: Web Inspector: Shortcuts screen UI polish
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: eustas.bug
URL: http://jtaby.com/2012/04/23/modern-we...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-24 04:43 PDT by eustas.bug
Modified: 2012-10-12 04:07 PDT (History)
12 users (show)

See Also:


Attachments
Screenshot: before patch (89.92 KB, image/png)
2012-04-26 05:43 PDT, eustas.bug
no flags Details
Screenshot: after patch (89.09 KB, image/png)
2012-04-26 05:43 PDT, eustas.bug
no flags Details
Patch (10.26 KB, patch)
2012-04-26 06:47 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (11.01 KB, patch)
2012-04-26 06:52 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (11.04 KB, patch)
2012-04-26 07:03 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (11.09 KB, patch)
2012-04-26 10:13 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (10.89 KB, patch)
2012-04-28 04:21 PDT, eustas.bug
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eustas.bug 2012-04-24 04:43:59 PDT
1) HUD have a different light source than the rest of the UI.
2) The border radius is too big.
3) The “X” button is not vertically centered in the header.
4) Some of the shortcuts are too small, so you can’t see them.
Comment 1 eustas.bug 2012-04-26 05:43:17 PDT
Created attachment 138981 [details]
Screenshot: before patch
Comment 2 eustas.bug 2012-04-26 05:43:59 PDT
Created attachment 138982 [details]
Screenshot: after patch
Comment 3 eustas.bug 2012-04-26 06:47:09 PDT
Created attachment 138989 [details]
Patch
Comment 4 eustas.bug 2012-04-26 06:52:19 PDT
Created attachment 138992 [details]
Patch
Comment 5 WebKit Review Bot 2012-04-26 06:54:50 PDT
Attachment 138992 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:15:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Pavel Feldman 2012-04-26 06:55:22 PDT
Comment on attachment 138989 [details]
Patch

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

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

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:82
> +        var minMax = totalHeight;

It is not clear what minMax stands for.

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:83
> +        var minMaxIdx = -1;

Please do not use abbreviations (should be Index)

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

for (var i = 0; i < ... ++i) {
}

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

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:106
> +            for (idx = stopIndices[columnIndex - 1]; idx < stopIndices[columnIndex]; ++idx) {

No need for { } around single line blocks.
Comment 7 eustas.bug 2012-04-26 07:03:56 PDT
Created attachment 138995 [details]
Patch
Comment 8 eustas.bug 2012-04-26 10:13:06 PDT
Created attachment 139019 [details]
Patch
Comment 9 eustas.bug 2012-04-26 10:18:00 PDT
Comment on attachment 138989 [details]
Patch

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.
Comment 10 Pavel Feldman 2012-04-28 00:29:42 PDT
Comment on attachment 139019 [details]
Patch

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

> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64
> +    Left: { code: 37, name: "<Left>" },           // also NUM_WEST

Even though they are not currently localized, they should be (at least Left, Right, Up and Down)

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:58
> +     * Renders sections to and appends them to the given node.

We don't comment on code in WebKit unless necessary.

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:110
> +};

No ; after }

> Source/WebCore/inspector/front-end/helpScreen.css:132
> +    color: #DD0;

We try to use rgb() notation for colors.
Comment 11 eustas.bug 2012-04-28 04:21:30 PDT
Created attachment 139353 [details]
Patch
Comment 12 eustas.bug 2012-04-28 04:24:14 PDT
Comment on attachment 139019 [details]
Patch

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

>> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64
>> +    Left: { code: 37, name: "<Left>" },           // also NUM_WEST
> 
> Even though they are not currently localized, they should be (at least Left, Right, Up and Down)

Done

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:58
>> +     * Renders sections to and appends them to the given node.
> 
> We don't comment on code in WebKit unless necessary.

Removed

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:110
>> +};
> 
> No ; after }

Rolled back here and anywhere it was added.

>> Source/WebCore/inspector/front-end/helpScreen.css:132
>> +    color: #DD0;
> 
> We try to use rgb() notation for colors.

fixed
Comment 13 WebKit Review Bot 2012-04-28 07:10:19 PDT
Comment on attachment 139353 [details]
Patch

Clearing flags on attachment: 139353

Committed r115568: <http://trac.webkit.org/changeset/115568>
Comment 14 WebKit Review Bot 2012-04-28 07:10:24 PDT
All reviewed patches have been landed.  Closing bug.