Bug 84708 - Web Inspector: Shortcuts screen UI polish
: Web Inspector: Shortcuts screen UI polish
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://jtaby.com/2012/04/23/modern-we...
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-24 04:43 PST by
Modified: 2012-10-12 04:07 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-24 04:43:59 PST
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 From 2012-04-26 05:43:17 PST -------
Created an attachment (id=138981) [details]
Screenshot: before patch
------- Comment #2 From 2012-04-26 05:43:59 PST -------
Created an attachment (id=138982) [details]
Screenshot: after patch
------- Comment #3 From 2012-04-26 06:47:09 PST -------
Created an attachment (id=138989) [details]
Patch
------- Comment #4 From 2012-04-26 06:52:19 PST -------
Created an attachment (id=138992) [details]
Patch
------- Comment #5 From 2012-04-26 06:54:50 PST -------
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 From 2012-04-26 06:55:22 PST -------
(From update of attachment 138989 [details])
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 From 2012-04-26 07:03:56 PST -------
Created an attachment (id=138995) [details]
Patch
------- Comment #8 From 2012-04-26 10:13:06 PST -------
Created an attachment (id=139019) [details]
Patch
------- Comment #9 From 2012-04-26 10:18:00 PST -------
(From update of attachment 138989 [details])
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 From 2012-04-28 00:29:42 PST -------
(From update of attachment 139019 [details])
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 From 2012-04-28 04:21:30 PST -------
Created an attachment (id=139353) [details]
Patch
------- Comment #12 From 2012-04-28 04:24:14 PST -------
(From update of attachment 139019 [details])
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 From 2012-04-28 07:10:19 PST -------
(From update of attachment 139353 [details])
Clearing flags on attachment: 139353

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