Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1902963002:
DevTools: Update styles for network conditions drawer panel (Closed)
|
Created:
4 years, 8 months ago by luoe Modified:
4 years, 8 months ago Reviewers:
lushnikov CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman, sergeyv+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate styles for network conditions drawer panel
BUG=604833
Committed: https://crrev.com/a3e2476b11f02ce6bae46599db3ac66b1a70fd22
Cr-Commit-Position: refs/heads/master@{#389017}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments 1 #Patch Set 3 : Added section separator styling #
Total comments: 14
Patch Set 4 : Address comments in patch 3 #Patch Set 5 : Address comments in patch 4 #Patch Set 6 : Remove unnecessary constant #
Messages
Total messages: 20 (7 generated)
Description was changed from ========== Update styles for network conditions drawer panel See design slides https://docs.google.com/presentation/d/1mmGYr6pqwEC74qudxUkdvKfpC3T4FupNIfz-G... BUG=604833 ========== to ========== Update styles for network conditions drawer panel BUG=604833 ==========
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
On 2016/04/19 at 21:12:47, luoe wrote: > This CL depends on another: https://codereview.chromium.org/1897383002 which makes the panel-separator CSS class available.
looks good! A few nits apart of inline comments: - we use "DevTools:" prefix in issue description. That helps to spot DevTools-related patches in patch lists - UI changes usually go with screenshots of the new design https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:18: this.contentElement.createChild("div").classList.add("panel-section-separator"); this is duplicate https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:101: for (var i = 0; i < groups.length; ++i) { I would avoid using i/j indexes here and use some more descriptive names for vars. Something like this: for (var userAgentDescriptor of WebInspector.NetworkConfigView._userAgentGroups) { var groupElement = userAgentselectElement.createChild("optgroup"); groupElement.label = userAgentDescriptor.title; for (var userAgentVersion of userAgentDescriptor.values) { .... } } https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:124: function indexOfAgent(agentValue) please add jsdoc https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:161: userAgentSelectElement.selectedIndex = indexOfAgent(WebInspector.NetworkConfigView.NonAgentValues.Custom); this function is called with Custom value as an argument only. Can we just store the index and use it here every time? https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:202: title: "Android", is this just a pretty-print? no changes here?
https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:18: this.contentElement.createChild("div").classList.add("panel-section-separator"); On 2016/04/19 at 22:28:08, lushnikov wrote: > this is duplicate I believe this duplicate is actually needed for a second separator between the 2nd and 3rd sections. https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:101: for (var i = 0; i < groups.length; ++i) { On 2016/04/19 at 22:28:08, lushnikov wrote: > I would avoid using i/j indexes here and use some more descriptive names for vars. > Something like this: > > for (var userAgentDescriptor of WebInspector.NetworkConfigView._userAgentGroups) { > var groupElement = userAgentselectElement.createChild("optgroup"); > groupElement.label = userAgentDescriptor.title; > for (var userAgentVersion of userAgentDescriptor.values) { > .... > } > } Sounds good https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:124: function indexOfAgent(agentValue) On 2016/04/19 at 22:28:08, lushnikov wrote: > please add jsdoc Will do https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:161: userAgentSelectElement.selectedIndex = indexOfAgent(WebInspector.NetworkConfigView.NonAgentValues.Custom); On 2016/04/19 at 22:28:08, lushnikov wrote: > this function is called with Custom value as an argument only. Can we just store the index and use it here every time? We sure can! https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:202: title: "Android", On 2016/04/19 at 22:28:08, lushnikov wrote: > is this just a pretty-print? no changes here? a few changes here: - alphabetic reordering - removal of Blackberry, MeeGo, Silk - addition of Edge for Xbox, Opera mobile, a few others
On 2016/04/19 22:58:43, luoe wrote: > https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... > File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js > (right): > > https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:18: > this.contentElement.createChild("div").classList.add("panel-section-separator"); > On 2016/04/19 at 22:28:08, lushnikov wrote: > > this is duplicate > > I believe this duplicate is actually needed for a second separator between the > 2nd and 3rd sections. > > https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:101: > for (var i = 0; i < groups.length; ++i) { > On 2016/04/19 at 22:28:08, lushnikov wrote: > > I would avoid using i/j indexes here and use some more descriptive names for > vars. > > Something like this: > > > > for (var userAgentDescriptor of > WebInspector.NetworkConfigView._userAgentGroups) { > > var groupElement = userAgentselectElement.createChild("optgroup"); > > groupElement.label = userAgentDescriptor.title; > > for (var userAgentVersion of userAgentDescriptor.values) { > > .... > > } > > } > > Sounds good > > https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:124: > function indexOfAgent(agentValue) > On 2016/04/19 at 22:28:08, lushnikov wrote: > > please add jsdoc > > Will do > > https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:161: > userAgentSelectElement.selectedIndex = > indexOfAgent(WebInspector.NetworkConfigView.NonAgentValues.Custom); > On 2016/04/19 at 22:28:08, lushnikov wrote: > > this function is called with Custom value as an argument only. Can we just > store the index and use it here every time? > > We sure can! > > https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... > third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:202: > title: "Android", > On 2016/04/19 at 22:28:08, lushnikov wrote: > > is this just a pretty-print? no changes here? > > a few changes here: > - alphabetic reordering > - removal of Blackberry, MeeGo, Silk > - addition of Edge for Xbox, Opera mobile, a few others Comments should be addressed, ready for review.
https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:101: for (var i = 0; i < groups.length; ++i) { On 2016/04/19 22:28:08, lushnikov wrote: > I would avoid using i/j indexes here and use some more descriptive names for > vars. > Something like this: > > for (var userAgentDescriptor of WebInspector.NetworkConfigView._userAgentGroups) > { > var groupElement = userAgentselectElement.createChild("optgroup"); > groupElement.label = userAgentDescriptor.title; > for (var userAgentVersion of userAgentDescriptor.values) { > .... > } > } Done. https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:124: function indexOfAgent(agentValue) On 2016/04/19 22:28:08, lushnikov wrote: > please add jsdoc Done. https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:161: userAgentSelectElement.selectedIndex = indexOfAgent(WebInspector.NetworkConfigView.NonAgentValues.Custom); On 2016/04/19 22:28:08, lushnikov wrote: > this function is called with Custom value as an argument only. Can we just store > the index and use it here every time? Done. https://codereview.chromium.org/1902963002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:202: title: "Android", On 2016/04/19 22:28:08, lushnikov wrote: > is this just a pretty-print? no changes here? Done.
https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:18: this.contentElement.createChild("div").classList.add("panel-section-separator"); this is a dupe line https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:105: for (var userAgentVersion of userAgentDescriptor.values) { style: omit {} we don't use {} for single-line blocks https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:111: var customAgentIndex = agentValues.indexOf(customOverrideValue); wait, isn't it always a zero? https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:112: userAgentSelectElement.selectedIndex = customOverrideValue; you probably wanted to assign selectedIndex to customAgentIndex? https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:126: stray line
https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:18: this.contentElement.createChild("div").classList.add("panel-section-separator"); On 2016/04/20 23:57:09, lushnikov wrote: > this is a dupe line The dupe is needed since we need two dividers. https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:105: for (var userAgentVersion of userAgentDescriptor.values) { On 2016/04/20 23:57:09, lushnikov wrote: > style: omit {} > > we don't use {} for single-line blocks Done. https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:111: var customAgentIndex = agentValues.indexOf(customOverrideValue); On 2016/04/20 23:57:09, lushnikov wrote: > wait, isn't it always a zero? Yes. If we ever reorder the options UI, move "custom" to the bottom of the list, it will still work. Is this extra logic unnecessary? https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:112: userAgentSelectElement.selectedIndex = customOverrideValue; On 2016/04/20 23:57:09, lushnikov wrote: > you probably wanted to assign selectedIndex to customAgentIndex? Done.
Thanks! lgtm given comments are addressed. https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:18: this.contentElement.createChild("div").classList.add("panel-section-separator"); On 2016/04/21 01:17:38, luoe wrote: > On 2016/04/20 23:57:09, lushnikov wrote: > > this is a dupe line > > The dupe is needed since we need two dividers. Thank you for clarification https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:111: var customAgentIndex = agentValues.indexOf(customOverrideValue); On 2016/04/21 01:17:38, luoe wrote: > On 2016/04/20 23:57:09, lushnikov wrote: > > wait, isn't it always a zero? > > Yes. If we ever reorder the options UI, move "custom" to the bottom of the list, > it will still work. Is this extra logic unnecessary? Yeah, i'd remove it unless it is needed. https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:126: On 2016/04/20 23:57:09, lushnikov wrote: > stray line can you please remove unneeded newline?
https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js (right): https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:111: var customAgentIndex = agentValues.indexOf(customOverrideValue); On 2016/04/21 17:10:30, lushnikov wrote: > On 2016/04/21 01:17:38, luoe wrote: > > On 2016/04/20 23:57:09, lushnikov wrote: > > > wait, isn't it always a zero? > > > > Yes. If we ever reorder the options UI, move "custom" to the bottom of the > list, > > it will still work. Is this extra logic unnecessary? > > Yeah, i'd remove it unless it is needed. Done. https://codereview.chromium.org/1902963002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/network/NetworkConfigView.js:126: On 2016/04/21 17:10:30, lushnikov wrote: > On 2016/04/20 23:57:09, lushnikov wrote: > > stray line > > can you please remove unneeded newline? Done.
Description was changed from ========== Update styles for network conditions drawer panel BUG=604833 ========== to ========== Update styles for network conditions drawer panel BUG=604833 ==========
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/1902963002/#ps100001 (title: "Remove unnecessary constant")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1902963002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1902963002/100001
Message was sent while issue was closed.
Description was changed from ========== Update styles for network conditions drawer panel BUG=604833 ========== to ========== Update styles for network conditions drawer panel BUG=604833 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Update styles for network conditions drawer panel BUG=604833 ========== to ========== Update styles for network conditions drawer panel BUG=604833 Committed: https://crrev.com/a3e2476b11f02ce6bae46599db3ac66b1a70fd22 Cr-Commit-Position: refs/heads/master@{#389017} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a3e2476b11f02ce6bae46599db3ac66b1a70fd22 Cr-Commit-Position: refs/heads/master@{#389017} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
