Summary: | Web Inspector: Shortcuts screen UI polish | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | eustas.bug | ||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | eustas.bug | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, eustas.bug, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
URL: | http://jtaby.com/2012/04/23/modern-web-development-part-1.html | ||||||||||||||||||
Attachments: |
|
Description
eustas.bug
2012-04-24 04:43:59 PDT
Created attachment 138981 [details]
Screenshot: before patch
Created attachment 138982 [details]
Screenshot: after patch
Created attachment 138989 [details]
Patch
Created attachment 138992 [details]
Patch
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 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. Created attachment 138995 [details]
Patch
Created attachment 139019 [details]
Patch
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 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. Created attachment 139353 [details]
Patch
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 on attachment 139353 [details] Patch Clearing flags on attachment: 139353 Committed r115568: <http://trac.webkit.org/changeset/115568> All reviewed patches have been landed. Closing bug. |