Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2178083002:
MD Settings: Network details cleanup (Closed)
|
Created:
4 years, 4 months ago by stevenjb Modified:
4 years, 4 months ago Reviewers:
dschuyler CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@issue_609156_internet_cleanup_2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Network details cleanup
This CL cleans up the internet detail page to better match
the mocks.
BUG=609156
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/36206446189aaa8ce607c0ffcc081542cfa23b16
Cr-Commit-Position: refs/heads/master@{#407946}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : WS cleanup #
Total comments: 10
Patch Set 4 : Rebase + Feedback #
Total comments: 4
Patch Set 5 : More feedback #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 15 (4 generated)
Description was changed from ========== MD Settings: Network details cleanup This CL cleans up the internet detail page to better match the mocks. BUG=609156 ========== to ========== MD Settings: Network details cleanup This CL cleans up the internet detail page to better match the mocks. BUG=609156 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dschuyler@chromium.org
A lot of the JS changes are due to clang formatting, apologies for the churn.
https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:75: <div class="settings-box first layout vertical"> nit: Is layout vertical necessary? https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:78: <div class="start layout horizontal center"> nit: Is layout horizontal center necessary? The idea behind the settings-box and start and such is that the layout * would not be needed. Does settings-box need something fixed? https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:112: This network is shared with other users. Question: Do these in-lined strings need i18n? Is there a bug already open for these? https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:675: isType_: function(properties, type) { return properties.Type == type; }, nit: I think it's wise to keep executable lines separate. It makes it easier to set breakpoints in a debugger.
Thanks for the quick review, PTAL. https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:75: <div class="settings-box first layout vertical"> On 2016/07/26 01:18:23, dschuyler wrote: > nit: Is layout vertical necessary? flex-direction defaults to 'row'. I can use the new 'settings-box.single-column' though instead of 'layout.vertical'. https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:78: <div class="start layout horizontal center"> On 2016/07/26 01:18:23, dschuyler wrote: > nit: Is layout horizontal center necessary? > > The idea behind the settings-box and start > and such is that the layout * would not be > needed. Does settings-box need something fixed? Changed to 'settings-box embedded' (I don't think we should try to completely avoid layout, 'settings-box' doesn't make sense everywhere, e.g. the divs below, but here it does). https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:112: This network is shared with other users. On 2016/07/26 01:18:23, dschuyler wrote: > Question: > > Do these in-lined strings need i18n? > > Is there a bug already open for these? Yes and yes. https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:675: isType_: function(properties, type) { return properties.Type == type; }, On 2016/07/26 01:18:23, dschuyler wrote: > nit: I think it's wise to keep executable lines separate. It makes it easier to > set breakpoints in a debugger. clang-format does this. It's a huge pita to format just format some bits by hand. Fortunately I believe the JS debugger does let you set a breakpoint on this line.
I'll say LGTM (with nits) if there is a way to set the breakpoint on the one line function (that I commented about). If there's not a way to do that, please wrap the line :) https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:675: isType_: function(properties, type) { return properties.Type == type; }, On 2016/07/26 16:47:17, stevenjb wrote: > On 2016/07/26 01:18:23, dschuyler wrote: > > nit: I think it's wise to keep executable lines separate. It makes it easier > to > > set breakpoints in a debugger. > > clang-format does this. It's a huge pita to format just format some bits by > hand. Fortunately I believe the JS debugger does let you set a breakpoint on > this line. I setup a similar function and tried to set a breakpoint on it. The breakpoint triggers during creation of the object (the Polymer({ ... }) that encloses the function member). I was unable to set a breakpoint on the 'return' line. Is there a trick to it that I'm missing? https://codereview.chromium.org/2178083002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/2178083002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:58: font-size: 100%; nit: can this font-size: 100%; be removed? It looks like this is only used on line 82. iiuc, it's effectively saying font-size = font-size * 1 If it is needed, it would help me learn to hear why it's needed :) https://codereview.chromium.org/2178083002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:192: <!-- IP Config --> nit: I'm of a mixed opinion on the comments in this file. Can we remove this last nesting of comments? (Where the comments are 1:1 with a thing with almost the same name as the comment). (Removing more is good too).
PTAL https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2178083002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.js:675: isType_: function(properties, type) { return properties.Type == type; }, On 2016/07/26 19:19:46, dschuyler wrote: > On 2016/07/26 16:47:17, stevenjb wrote: > > On 2016/07/26 01:18:23, dschuyler wrote: > > > nit: I think it's wise to keep executable lines separate. It makes it easier > > to > > > set breakpoints in a debugger. > > > > clang-format does this. It's a huge pita to format just format some bits by > > hand. Fortunately I believe the JS debugger does let you set a breakpoint on > > this line. > > I setup a similar function and tried to set a breakpoint on it. The > breakpoint triggers during creation of the object (the Polymer({ ... }) that > encloses the function member). I was unable to set a breakpoint > on the 'return' line. Is there a trick to it that I'm missing? Ugh, you're right, I swear that used to work. After some experimentation, I discovered that I can add the following to a .clang-format file in settings/ directory: BasedOnStyle: Chromium AllowShortFunctionsOnASingleLine: None I will discuss on Friday's standup whether we should commit this (I'm missing Wednesdays). https://codereview.chromium.org/2178083002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/2178083002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:58: font-size: 100%; On 2016/07/26 19:19:47, dschuyler wrote: > nit: can this font-size: 100%; be removed? > It looks like this is only used on line 82. > iiuc, it's effectively saying > font-size = font-size * 1 > > If it is needed, it would help me learn to > hear why it's needed :) It is not strictly needed. I was playing with these values, they aren't final. I'm actually going to change this to 125% and I'll remove the networkState value (changing it to 100%). I need to get Alan to make a call. I also realized these can be <div> https://codereview.chromium.org/2178083002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:192: <!-- IP Config --> On 2016/07/26 19:19:47, dschuyler wrote: > nit: I'm of a mixed opinion on the comments > in this file. Can we remove this last nesting > of comments? (Where the comments are 1:1 with > a thing with almost the same name as the comment). > > (Removing more is good too). I hear what you are saying, but the comments are *much* easier for me to read in my editor, and this layout is a bit complicated.
LGTM (but with another suggestion based on the span to div changes). https://codereview.chromium.org/2178083002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/2178083002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:81: <div class="layout vertical"> nit: Nice, now that the sub-nodes are divs the "layout vertical" may be unnecessary :)
https://codereview.chromium.org/2178083002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/internet_detail_page.html (right): https://codereview.chromium.org/2178083002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/internet_detail_page.html:81: <div class="layout vertical"> On 2016/07/26 20:33:19, dschuyler wrote: > nit: Nice, now that the sub-nodes are divs > the "layout vertical" may be unnecessary :) Hm, seems you're correct. Sigh, I hate HTML layout. I think I am going to keep this as-is for readability.
The CQ bit was checked by stevenjb@chromium.org
P.S. Thanks for the detailed review!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Network details cleanup This CL cleans up the internet detail page to better match the mocks. BUG=609156 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Network details cleanup This CL cleans up the internet detail page to better match the mocks. BUG=609156 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/36206446189aaa8ce607c0ffcc081542cfa23b16 Cr-Commit-Position: refs/heads/master@{#407946} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/36206446189aaa8ce607c0ffcc081542cfa23b16 Cr-Commit-Position: refs/heads/master@{#407946} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
