Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1100993002:
Implement md-settings network lists. (Closed)
|
Created:
5 years, 8 months ago by stevenjb Modified:
5 years, 7 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement md-settings network lists.
* Adds cr-network-list and cr-expand-button
* Adds network-summary-item containing expandable network list
* Implements 'connect' functionality for selected networks
BUG=475667
Committed: https://crrev.com/9b80af9d3cc30689b982b5d627ca0f58a1dd473a
Cr-Commit-Position: refs/heads/master@{#327224}
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 84
Patch Set 3 : Feedback #Patch Set 4 : More Feedback, fix @type and @param #Patch Set 5 : Request network states on expand wifi #
Total comments: 6
Patch Set 6 : Rename device-enabled-toggled, fix disabled network text #
Total comments: 16
Patch Set 7 : Rebase + Feedback #Patch Set 8 : Rebase #Messages
Total messages: 22 (4 generated)
stevenjb@chromium.org changed reviewers: + jlklein@chromium.org, michaelpg@chromium.org
This CL introduces a good chunk of the UI features, even though there is only a limited amount of functionality.
Good stuff. Couple meta comments: * your description says you add cr-network-list-item, but it already exists * bug? the (n)th time a list expands, the expand happens instantaneously instead of gradually, for n > 1 (but this was over remote desktop so it may be wonky) https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.html (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.html:10: <cr-network-summary-item Is there any way you can simplify this with a repeated template or something? I understand the desire for declarative layout but this is kinda unintelligible to me and makes bugs hard to spot. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:169: * @param {{detail: CrOncDataElement}} event !CrOncDataElement https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:150: * Event triggered the enable button is toggled. nit: "when" https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:2: <link rel="import" href="chrome://resources/polymer/paper-button/paper-button.html"> import core-icon as well https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:6: <paper-button id="expand" disabled="{{disabled}}" paper-button (through CoreFocusable) has a "toggle" attribute which will make the "active" attribute do what you currently do for "expanded" -- so you might be able to just set <paper-button toggle active="{{expanded}}" disabled="{{disabled}}"> and not have to manually wire up click click events. See "Toggle buttons" in https://www.polymer-project.org/0.5/components/paper-button/demo.html https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:6: <paper-button id="expand" disabled="{{disabled}}" If there's no need to give the button an ID, let's leave it out. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.js:12: * <cr-expand-button expanded={{sectionIsExpanded}}></cr-expand-button> nit: need "" around the {{}} in the example https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.html:15: networkState="{{model}}" isListItem="{{true}}"> I believe the isListItem attribute can be used standalone if it's always true. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:31: networks: [], @default null?
On 2015/04/23 02:16:29, michaelpg wrote: > Good stuff. Couple meta comments: > > * your description says you add cr-network-list-item, but it already exists Fixed. > * bug? the (n)th time a list expands, the expand happens instantaneously instead > of gradually, for n > 1 (but this was over remote desktop so it may be wonky) I've seen this sometimes too. Seems to be a bug in core-collapse (or cr-collapse) maybe? I figure we can deal with that sort of polish issue once we switch to Polymer .8.
Mostly nits. Nice Job. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.html (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.html:4: <link rel="import" href="network_summary_style.html"> Missing core-style import. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.html:13: maxHeight="200" This seems like something that should go in the css. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:50: /** @const {Array<string>} */ var NETWORK_TYPES = nit: I think this would read cleaner like: /** @const {!Array<string>} */ var NETWORK_TYPES = ['Ethernet', ... https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:155: itemIsVisible: function(deviceState) { return !!deviceState; }, Trailing _. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:162: onExpanded: function(event) { Trailing _. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:163: if (event.detail.type == 'WiFi') Why is this needed if this function is only ever called when the wifi item is expanded? Why not just call it onWifiExpanded? https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:172: onSelected: function(event) { Trailing _. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:186: onToggleEnabled: function(event) { @private? Also nit: maybe just onToggled since this is also called when the toggle is disabled. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:197: onNetworkListChangedEvent_: function() { this.getNetworks_(); }, It doesn't seem like this function and onDeviceStateListChangedEvent are necessary. Why not just bind directly to this.getNetworks_ in both cases? https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:222: * @param {CrOncDataElement} state The network state. !CrOncDataElement https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:30: * @default 1000 If all actual uses are 200, why not make that the default? https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:87: * @param {DeviceStateProperties} deviceState The state of a device. nit: For nullable params here and elsewhere, please add a leading ? to be explicit. See: https://docs.google.com/document/d/1Guyc0gizRp3Qd5i0LHXQ7uMaowfEvvXe-tY7hTM5V... https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:88: * @private nit here and below: Our convention most places is to put @private last. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:91: deviceIsEnabled: function(deviceState) { Here and elsewhere: trailing _ on private functions. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:107: * @param {Array<CrOncDataElement>|undefined} netowrkList A list of networks. s/netowrkList/networkList/ https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:107: * @param {Array<CrOncDataElement>|undefined} netowrkList A list of networks. Can the networkList be null or undefined? If it can only be undefined, you can add a '!'. Also if the CrOncDataElements are non-null, add a '!'. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:136: event.stopPropagation(); Why do we need to stop propagation in this case? https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:165: // TODO(stevenjb): Make any actionalble item selectable. "actionable" https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item_style.html (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item_style.html:11: flex: 1 1 auto; Isn't this equivalent to flex: 1? You can also just add a "flex" attribute to the detailsItem element. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:6: <paper-button id="expand" disabled="{{disabled}}" It seems like core-icon-button or paper-icon-button is a better fit here. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.css (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.css:7: display: flex; This and the next two rules could be: layout vertical flex in the template. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.css:16: flex: 1 1 auto; Just add a flex attribute to the core-list element https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:19: * @type number nit: Wrap types in {} https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:20: * @default 1000 Why not 200? https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:64: event.stopPropagation(); Why stopPropagation here? https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js:99: this.$.divOuter.classList.toggle('listItem', this.isListItem); I feel like we should be able to just do this via a data binding in the template without touching the js.
On 2015/04/23 16:43:29, stevenjb wrote: > On 2015/04/23 02:16:29, michaelpg wrote: > > Good stuff. Couple meta comments: > > > > * your description says you add cr-network-list-item, but it already exists > Fixed. > > > * bug? the (n)th time a list expands, the expand happens instantaneously > instead > > of gradually, for n > 1 (but this was over remote desktop so it may be wonky) > I've seen this sometimes too. Seems to be a bug in core-collapse (or > cr-collapse) maybe? I figure we can deal with that sort of polish issue once we > switch to Polymer .8. +1 to not worrying too much about this until the 0.8 switch. It's weird I can't repro in the core-collapse demo though: https://www.polymer-project.org/0.5/components/core-collapse/demo.html I think it could be a timing/sizing issue with the list. Regardless, holding off on this sort of thing until 0.8 seems reasonable to me.
PTAL https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.html (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.html:4: <link rel="import" href="network_summary_style.html"> On 2015/04/23 18:21:14, Jeremy Klein wrote: > Missing core-style import. Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.html:10: <cr-network-summary-item On 2015/04/23 02:16:29, michaelpg wrote: > Is there any way you can simplify this with a repeated template or something? I > understand the desire for declarative layout but this is kinda unintelligible to > me and makes bugs hard to spot. I'm not familiar enough with html templates to say for sure, but it seems like any such approach would obscure the data bindings. I agree that it is a bit verbose, but I am reluctant to add too much complexity for the sake of brevity. That said, I am open to specific suggestions. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.html:13: maxHeight="200" On 2015/04/23 18:21:14, Jeremy Klein wrote: > This seems like something that should go in the css. That doesn't appear to work for Polymer attributes. However I removed this and changed the default for cr-network-summary-item. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:50: /** @const {Array<string>} */ var NETWORK_TYPES = On 2015/04/23 18:21:15, Jeremy Klein wrote: > nit: I think this would read cleaner like: > > /** @const {!Array<string>} */ > var NETWORK_TYPES = ['Ethernet', ... Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:155: itemIsVisible: function(deviceState) { return !!deviceState; }, On 2015/04/23 18:21:14, Jeremy Klein wrote: > Trailing _. I haven't use a trailing _ in methods that are used in data binding in the HTML because it looks odd to me, but I don't feel strongly either way. We should be consistent either way throughout Settings, so we should pick an approach and stick with it. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:162: onExpanded: function(event) { On 2015/04/23 18:21:15, Jeremy Klein wrote: > Trailing _. Acknowledged. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:163: if (event.detail.type == 'WiFi') On 2015/04/23 18:21:14, Jeremy Klein wrote: > Why is this needed if this function is only ever called when the wifi item is > expanded? Why not just call it onWifiExpanded? In the future we may do things for other types, but we can add new methods for those, so I'll rename this and remove the if. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:169: * @param {{detail: CrOncDataElement}} event On 2015/04/23 02:16:29, michaelpg wrote: > !CrOncDataElement Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:172: onSelected: function(event) { On 2015/04/23 18:21:14, Jeremy Klein wrote: > Trailing _. Acknowledged. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:186: onToggleEnabled: function(event) { On 2015/04/23 18:21:14, Jeremy Klein wrote: > @private? > > Also nit: maybe just onToggled since this is also called when the toggle is > disabled. Done. Well, it's toggling the "enabled" state (vs. e.g. the visibility of the list), so I think the current name is maybe slightly less confusing? https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:197: onNetworkListChangedEvent_: function() { this.getNetworks_(); }, On 2015/04/23 18:21:15, Jeremy Klein wrote: > It doesn't seem like this function and onDeviceStateListChangedEvent are > necessary. Why not just bind directly to this.getNetworks_ in both cases? While currently true, I kind of prefer to keep these separate because: a) I think it makes the asynchronous nature of the callback a little more clear. b) We may do more in these later (which I realize we could change at that time, but see 'a'). https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:222: * @param {CrOncDataElement} state The network state. On 2015/04/23 18:21:14, Jeremy Klein wrote: > !CrOncDataElement Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:30: * @default 1000 On 2015/04/23 18:21:15, Jeremy Klein wrote: > If all actual uses are 200, why not make that the default? I guess that's fine here. I didn't want to bake that into cr_network_list, but since this is already specific to this page, that seems fine. Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:87: * @param {DeviceStateProperties} deviceState The state of a device. On 2015/04/23 18:21:15, Jeremy Klein wrote: > nit: For nullable params here and elsewhere, please add a leading ? to be > explicit. > > See: > https://docs.google.com/document/d/1Guyc0gizRp3Qd5i0LHXQ7uMaowfEvvXe-tY7hTM5V... Ugh. Michael suggested elsewhere that ? wasn't necessary for objects. While ! makes sense, having ? everywhere seems tedious to me. Either way, we should come to an agreement on this. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:88: * @private On 2015/04/23 18:21:15, Jeremy Klein wrote: > nit here and below: Our convention most places is to put @private last. Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:91: deviceIsEnabled: function(deviceState) { On 2015/04/23 18:21:15, Jeremy Klein wrote: > Here and elsewhere: trailing _ on private functions. I'll change all data binding methods if we decide to do so universally. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:107: * @param {Array<CrOncDataElement>|undefined} netowrkList A list of networks. On 2015/04/23 18:21:15, Jeremy Klein wrote: > Can the networkList be null or undefined? If it can only be undefined, you can > add a '!'. Also if the CrOncDataElements are non-null, add a '!'. It can be null, not undefined. Fixed. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:136: event.stopPropagation(); On 2015/04/23 18:21:15, Jeremy Klein wrote: > Why do we need to stop propagation in this case? Probably here we don't, but I got bit by the buttons within the div and I decided I would rather be safe throughout. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:150: * Event triggered the enable button is toggled. On 2015/04/23 02:16:29, michaelpg wrote: > nit: "when" Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:165: // TODO(stevenjb): Make any actionalble item selectable. On 2015/04/23 18:21:15, Jeremy Klein wrote: > "actionable" Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item_style.html (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item_style.html:11: flex: 1 1 auto; On 2015/04/23 18:21:15, Jeremy Klein wrote: > Isn't this equivalent to flex: 1? You can also just add a "flex" attribute to > the detailsItem element. Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:2: <link rel="import" href="chrome://resources/polymer/paper-button/paper-button.html"> On 2015/04/23 02:16:29, michaelpg wrote: > import core-icon as well Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:6: <paper-button id="expand" disabled="{{disabled}}" On 2015/04/23 18:21:15, Jeremy Klein wrote: > It seems like core-icon-button or paper-icon-button is a better fit here. paper-icon-button only has a single icon. Using 'active' (not mentioned in paper-button docs, thankyouverymuch) highlights the button, which looks a little odd, but we can probably change that with styling, and will give the UX folks more to think about :) Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.js:12: * <cr-expand-button expanded={{sectionIsExpanded}}></cr-expand-button> On 2015/04/23 02:16:29, michaelpg wrote: > nit: need "" around the {{}} in the example Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.css (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.css:7: display: flex; On 2015/04/23 18:21:15, Jeremy Klein wrote: > This and the next two rules could be: > > layout vertical flex > > in the template. Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.css:16: flex: 1 1 auto; On 2015/04/23 18:21:15, Jeremy Klein wrote: > Just add a flex attribute to the core-list element Interestingly that doesn't work. I suspect it may be related to core-list needing to have a defined height. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.html:15: networkState="{{model}}" isListItem="{{true}}"> On 2015/04/23 02:16:29, michaelpg wrote: > I believe the isListItem attribute can be used standalone if it's always true. Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:19: * @type number On 2015/04/23 18:21:15, Jeremy Klein wrote: > nit: Wrap types in {} Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:20: * @default 1000 On 2015/04/23 18:21:15, Jeremy Klein wrote: > Why not 200? Here I don't want to impose any specific layout sizing in an element; that should be a property of the page. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:31: networks: [], On 2015/04/23 02:16:29, michaelpg wrote: > @default null? Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:64: event.stopPropagation(); On 2015/04/23 18:21:15, Jeremy Klein wrote: > Why stopPropagation here? Here it probably doesn't make sense to do this. Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js:99: this.$.divOuter.classList.toggle('listItem', this.isListItem); On 2015/04/23 18:21:16, Jeremy Klein wrote: > I feel like we should be able to just do this via a data binding in the template > without touching the js. I am open to suggestions as to how to do that...
https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:155: itemIsVisible: function(deviceState) { return !!deviceState; }, On 2015/04/24 01:25:25, stevenjb wrote: > On 2015/04/23 18:21:14, Jeremy Klein wrote: > > Trailing _. > > I haven't use a trailing _ in methods that are used in data binding in the HTML > because it looks odd to me, but I don't feel strongly either way. We should be > consistent either way throughout Settings, so we should pick an approach and > stick with it. This is part of the google style guide and I think either the compiler or linter will complain if you have @private and no trailing underscore. Please add it everywhere. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:197: onNetworkListChangedEvent_: function() { this.getNetworks_(); }, On 2015/04/24 01:25:25, stevenjb wrote: > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > It doesn't seem like this function and onDeviceStateListChangedEvent are > > necessary. Why not just bind directly to this.getNetworks_ in both cases? > > While currently true, I kind of prefer to keep these separate because: > a) I think it makes the asynchronous nature of the callback a little more clear. > b) We may do more in these later (which I realize we could change at that time, > but see 'a'). I still don't agree with this, but won't block on it. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:87: * @param {DeviceStateProperties} deviceState The state of a device. On 2015/04/24 01:25:25, stevenjb wrote: > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > nit: For nullable params here and elsewhere, please add a leading ? to be > > explicit. > > > > See: > > > https://docs.google.com/document/d/1Guyc0gizRp3Qd5i0LHXQ7uMaowfEvvXe-tY7hTM5V... > > Ugh. Michael suggested elsewhere that ? wasn't necessary for objects. While ! > makes sense, having ? everywhere seems tedious to me. Either way, we should come > to an agreement on this. ?s are strongly encouraged in Google3 by John Lenz so I'm definitely in favor of putting them in to be explicit. Otherwise you may have just forgotten a '!' (which happens pretty often). That being said, this isn't in the style guide so I won't block on it. Just my two-cents. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:136: event.stopPropagation(); On 2015/04/24 01:25:25, stevenjb wrote: > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > Why do we need to stop propagation in this case? > > Probably here we don't, but I got bit by the buttons within the div and I > decided I would rather be safe throughout. We really shouldn't be adding unnecessary stopPropagations because it breaks the normal expected event bubbling process. I've seen some really hard to track down bugs caused by this. We should have a very good clear reason for all of these. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js:99: this.$.divOuter.classList.toggle('listItem', this.isListItem); On 2015/04/24 01:25:26, stevenjb wrote: > On 2015/04/23 18:21:16, Jeremy Klein wrote: > > I feel like we should be able to just do this via a data binding in the > template > > without touching the js. > > I am open to suggestions as to how to do that... Can't you just use tokenList like you're doing for the class elsewhere? You can just bind it to isListItem right? https://www.polymer-project.org/0.5/docs/polymer/expressions.html#tokenlist
PTAL michaelpg@ - note: I added some test code in fake_shill_manager_client.cc. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:155: itemIsVisible: function(deviceState) { return !!deviceState; }, On 2015/04/24 02:55:28, Jeremy Klein wrote: > On 2015/04/24 01:25:25, stevenjb wrote: > > On 2015/04/23 18:21:14, Jeremy Klein wrote: > > > Trailing _. > > > > I haven't use a trailing _ in methods that are used in data binding in the > HTML > > because it looks odd to me, but I don't feel strongly either way. We should be > > consistent either way throughout Settings, so we should pick an approach and > > stick with it. > > This is part of the google style guide and I think either the compiler or linter > will complain if you have @private and no trailing underscore. Please add it > everywhere. Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:197: onNetworkListChangedEvent_: function() { this.getNetworks_(); }, On 2015/04/24 02:55:28, Jeremy Klein wrote: > On 2015/04/24 01:25:25, stevenjb wrote: > > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > > It doesn't seem like this function and onDeviceStateListChangedEvent are > > > necessary. Why not just bind directly to this.getNetworks_ in both cases? > > > > While currently true, I kind of prefer to keep these separate because: > > a) I think it makes the asynchronous nature of the callback a little more > clear. > > b) We may do more in these later (which I realize we could change at that > time, > > but see 'a'). > > I still don't agree with this, but won't block on it. Acknowledged. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:87: * @param {DeviceStateProperties} deviceState The state of a device. On 2015/04/24 02:55:28, Jeremy Klein wrote: > On 2015/04/24 01:25:25, stevenjb wrote: > > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > > nit: For nullable params here and elsewhere, please add a leading ? to be > > > explicit. > > > > > > See: > > > > > > https://docs.google.com/document/d/1Guyc0gizRp3Qd5i0LHXQ7uMaowfEvvXe-tY7hTM5V... > > > > Ugh. Michael suggested elsewhere that ? wasn't necessary for objects. While ! > > makes sense, having ? everywhere seems tedious to me. Either way, we should > come > > to an agreement on this. > > ?s are strongly encouraged in Google3 by John Lenz so I'm definitely in favor of > putting them in to be explicit. Otherwise you may have just forgotten a '!' > (which happens pretty often). That being said, this isn't in the style guide so > I won't block on it. Just my two-cents. Done. https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:136: event.stopPropagation(); On 2015/04/24 02:55:29, Jeremy Klein wrote: > On 2015/04/24 01:25:25, stevenjb wrote: > > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > > Why do we need to stop propagation in this case? > > > > Probably here we don't, but I got bit by the buttons within the div and I > > decided I would rather be safe throughout. > > We really shouldn't be adding unnecessary stopPropagations because it breaks the > normal expected event bubbling process. I've seen some really hard to track down > bugs caused by this. We should have a very good clear reason for all of these. Done. https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js:99: this.$.divOuter.classList.toggle('listItem', this.isListItem); On 2015/04/24 02:55:29, Jeremy Klein wrote: > On 2015/04/24 01:25:26, stevenjb wrote: > > On 2015/04/23 18:21:16, Jeremy Klein wrote: > > > I feel like we should be able to just do this via a data binding in the > > template > > > without touching the js. > > > > I am open to suggestions as to how to do that... > > Can't you just use tokenList like you're doing for the class elsewhere? You can > just bind it to isListItem right? > > https://www.polymer-project.org/0.5/docs/polymer/expressions.html#tokenlist Oh, right, I'd already forgotten about tokenList. Eventually all these data binding tricks will sink in. Done.
lgtm https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:186: onToggleEnabled: function(event) { On 2015/04/24 01:25:25, stevenjb wrote: > On 2015/04/23 18:21:14, Jeremy Klein wrote: > > @private? > > > > Also nit: maybe just onToggled since this is also called when the toggle is > > disabled. > Done. > > Well, it's toggling the "enabled" state (vs. e.g. the visibility of the list), > so I think the current name is maybe slightly less confusing? I think it's confusing too -- it implies the event is triggered when "toggle" is enabled. It sounds kind of silly but "onEnabledToggled" makes it clear to me what the event represents and follows the same sort of grammar other events use. onSelected: [when] this [was] selected onNetworkListChangedEvent: [when] network list [was] changed onEnabledToggled: [when] enabled [was] toggled https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:87: * @param {DeviceStateProperties} deviceState The state of a device. On 2015/04/24 02:55:28, Jeremy Klein wrote: > On 2015/04/24 01:25:25, stevenjb wrote: > > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > > nit: For nullable params here and elsewhere, please add a leading ? to be > > > explicit. > > > > > > See: > > > > > > https://docs.google.com/document/d/1Guyc0gizRp3Qd5i0LHXQ7uMaowfEvvXe-tY7hTM5V... > > > > Ugh. Michael suggested elsewhere that ? wasn't necessary for objects. While ! > > makes sense, having ? everywhere seems tedious to me. Either way, we should > come > > to an agreement on this. > > ?s are strongly encouraged in Google3 by John Lenz so I'm definitely in favor of > putting them in to be explicit. Otherwise you may have just forgotten a '!' > (which happens pretty often). That being said, this isn't in the style guide so > I won't block on it. Just my two-cents. We are not in google3 and we should not be referencing internal Google documents like this. I mentioned ? wasn't necessary for objects because it's not, and because it's not consistent with how we annotate the rest of Chrome JS. You could make the same argument for including ! for all non-nullable types including booleans, numbers, strings and functions. That said, this isn't the hill I want to die on :-) https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:6: <paper-button id="expand" disabled="{{disabled}}" On 2015/04/24 01:25:26, stevenjb wrote: > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > It seems like core-icon-button or paper-icon-button is a better fit here. > > paper-icon-button only has a single icon. > > Using 'active' (not mentioned in paper-button docs, thankyouverymuch) highlights > the button, which looks a little odd, but we can probably change that with > styling, and will give the UX folks more to think about :) Done. > > Yeah, you have to click through the whole inheritance chain because they don't show "inherited" attributes for mixins :-( But I'm glad we made this work. https://codereview.chromium.org/1100993002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:248: * Requests the list of network states from Chrome. Updates networkStates, and Remove the comma, so it's clear the callback does both things. https://codereview.chromium.org/1100993002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:259: filter, this.getNetworksCallback_.bind(this)); nit 4 spaces https://codereview.chromium.org/1100993002/diff/80001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/1100993002/diff/80001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_manager_client.cc:34: int s_extra_wifi_networks = 0; out of curiosity what is this s_ prefix? "Static"?
https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary_item.js:87: * @param {DeviceStateProperties} deviceState The state of a device. On 2015/04/24 19:35:37, michaelpg wrote: > On 2015/04/24 02:55:28, Jeremy Klein wrote: > > On 2015/04/24 01:25:25, stevenjb wrote: > > > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > > > nit: For nullable params here and elsewhere, please add a leading ? to be > > > > explicit. > > > > > > > > See: > > > > > > > > > > https://docs.google.com/document/d/1Guyc0gizRp3Qd5i0LHXQ7uMaowfEvvXe-tY7hTM5V... > > > > > > Ugh. Michael suggested elsewhere that ? wasn't necessary for objects. While > ! > > > makes sense, having ? everywhere seems tedious to me. Either way, we should > > come > > > to an agreement on this. > > > > ?s are strongly encouraged in Google3 by John Lenz so I'm definitely in favor > of > > putting them in to be explicit. Otherwise you may have just forgotten a '!' > > (which happens pretty often). That being said, this isn't in the style guide > so > > I won't block on it. Just my two-cents. > > We are not in google3 and we should not be referencing internal Google documents > like this. > > I mentioned ? wasn't necessary for objects because it's not, and because it's > not consistent with how we annotate the rest of Chrome JS. You could make the > same argument for including ! for all non-nullable types including booleans, > numbers, strings and functions. > > That said, this isn't the hill I want to die on :-) Enforcing ! for non-nullable types isn't exactly the same because it's less likely for there to be mistakes as a result of forgetting a ?. Worst case, if something *should* be nullable and isn't marked as such, the user would get a compile-time error when a nullable value is passed to that function. Accidentally leaving off a '!' for Objects is worse because it means you'll get *less* checking from the compiler and could accidentally pass null in a case where you don't want it. Does that make sense?
PTAL (note: this also includes a small change to properly display 'Disabled' text for disabled networks). https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:186: onToggleEnabled: function(event) { On 2015/04/24 19:35:37, michaelpg wrote: > On 2015/04/24 01:25:25, stevenjb wrote: > > On 2015/04/23 18:21:14, Jeremy Klein wrote: > > > @private? > > > > > > Also nit: maybe just onToggled since this is also called when the toggle is > > > disabled. > > Done. > > > > Well, it's toggling the "enabled" state (vs. e.g. the visibility of the list), > > so I think the current name is maybe slightly less confusing? > > I think it's confusing too -- it implies the event is triggered when "toggle" is > enabled. > > It sounds kind of silly but "onEnabledToggled" makes it clear to me what the > event represents and follows the same sort of grammar other events use. > > onSelected: [when] this [was] selected > onNetworkListChangedEvent: [when] network list [was] changed > onEnabledToggled: [when] enabled [was] toggled OK, I made this as explicit as I felt I reasonably should. Button -> deviceEnabledButton Event -> device-enabled-toggled Function -> onDeviceEnabledToggled_ (here and in network_summary_item) https://codereview.chromium.org/1100993002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:248: * Requests the list of network states from Chrome. Updates networkStates, and On 2015/04/24 19:35:37, michaelpg wrote: > Remove the comma, so it's clear the callback does both things. Done. https://codereview.chromium.org/1100993002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/network_summary.js:259: filter, this.getNetworksCallback_.bind(this)); On 2015/04/24 19:35:37, michaelpg wrote: > nit 4 spaces Done. https://codereview.chromium.org/1100993002/diff/80001/chromeos/dbus/fake_shil... File chromeos/dbus/fake_shill_manager_client.cc (right): https://codereview.chromium.org/1100993002/diff/80001/chromeos/dbus/fake_shil... chromeos/dbus/fake_shill_manager_client.cc:34: int s_extra_wifi_networks = 0; On 2015/04/24 19:35:37, michaelpg wrote: > out of curiosity what is this s_ prefix? "Static"? Yeah. Personal style thing.
lgtm With nits. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/network_summary.html (right): https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.html:20: hidden?="{{!itemIsVisible_(deviceStates.WiFi)}}" Looking at this again, I feel like maybe this should be encapsulated in cr-network-summary item since it already has deviceState. Is there any use of cr-network-summary-item where we don't want this hidden functionality? https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.js:51: NETWORK_TYPES = ['Ethernet', 'WiFi', 'Cellular', 'WiMAX', 'VPN']; nit: var on the next line. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.js:209: * @param {Array<string>} networkIds The list of changed network GUIDs. !Array? https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.js:305: /** @type {CrOncDataElement} */ var defaultState = null; Is this really the right type? It seems like the compile would fail on line 307. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary_item.js:107: * @param {?Array<!CrOncDataElement>} netwprkList A list of networks. "networkList" https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js (right): https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:22: maxHeight: 1000, nit: Is this ever changed anywhere? If not, can we just get rid of it and do this in the css for simplicity? https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:31: networks: [], Array initialization should be in the created callback like for Objects. https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js (right): https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js:68: * @default false nit: Isn't true the much more common case? I feel like this is a bit confusing to have an "isListItem" property on a CrNetworkListItem element. It would be more clear if this was just isStandAlone instead.
https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:6: <paper-button id="expand" disabled="{{disabled}}" On 2015/04/24 01:25:26, stevenjb wrote: > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > It seems like core-icon-button or paper-icon-button is a better fit here. > > paper-icon-button only has a single icon. > I'm not suggesting we get rid of this wrapper, just that we wrap paper-icon-button instead of paper-button and core-icon. paper-icon-button essentially does exactly what you're doing here and you could use the same exact binding in the icon attribute.
https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html (right): https://codereview.chromium.org/1100993002/diff/20001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_expand_button/cr_expand_button.html:6: <paper-button id="expand" disabled="{{disabled}}" On 2015/04/27 22:36:10, Jeremy Klein wrote: > On 2015/04/24 01:25:26, stevenjb wrote: > > On 2015/04/23 18:21:15, Jeremy Klein wrote: > > > It seems like core-icon-button or paper-icon-button is a better fit here. > > > > paper-icon-button only has a single icon. > > > > I'm not suggesting we get rid of this wrapper, just that we wrap > paper-icon-button instead of paper-button and core-icon. paper-icon-button > essentially does exactly what you're doing here and you could use the same exact > binding in the icon attribute. > Ah. Yes, that works, and we get a cute round hilight instead of a squarish one. Done. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/network_summary.html (right): https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.html:20: hidden?="{{!itemIsVisible_(deviceStates.WiFi)}}" On 2015/04/27 22:29:03, Jeremy Klein wrote: > Looking at this again, I feel like maybe this should be encapsulated in > cr-network-summary item since it already has deviceState. Is there any use of > cr-network-summary-item where we don't want this hidden functionality? For a more generic element I would argue that the page should control the visibility of its elements, but since this is already a special purpose element I guess I am fine with pushing this down to the element. Done. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/network_summary.js (right): https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.js:51: NETWORK_TYPES = ['Ethernet', 'WiFi', 'Cellular', 'WiMAX', 'VPN']; On 2015/04/27 22:29:03, Jeremy Klein wrote: > nit: var on the next line. Done. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.js:209: * @param {Array<string>} networkIds The list of changed network GUIDs. On 2015/04/27 22:29:03, Jeremy Klein wrote: > !Array? Done. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary.js:305: /** @type {CrOncDataElement} */ var defaultState = null; On 2015/04/27 22:29:03, Jeremy Klein wrote: > Is this really the right type? It seems like the compile would fail on line 307. Good catch, it should be NetworkStateProperties, and it needs a GUID (GUID and Type are the only required properties). Done. https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/network_summary_item.js (right): https://codereview.chromium.org/1100993002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/network_summary_item.js:107: * @param {?Array<!CrOncDataElement>} netwprkList A list of networks. On 2015/04/27 22:29:03, Jeremy Klein wrote: > "networkList" Done. https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js (right): https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:22: maxHeight: 1000, On 2015/04/27 22:29:03, Jeremy Klein wrote: > nit: Is this ever changed anywhere? If not, can we just get rid of it and do > this in the css for simplicity? Yes, it can (and probably should) be changed by whatever owns it. In this case, cr-network-summary-item, which sets it to 200 (for now, we'll tune that later). Other UI (e.g. the OOBE UI) may also end up using this element, and may want to size it differently, which is why we expose the attribute. https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_network_list/cr_network_list.js:31: networks: [], On 2015/04/27 22:29:03, Jeremy Klein wrote: > Array initialization should be in the created callback like for Objects. Done. https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js (right): https://codereview.chromium.org/1100993002/diff/100001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/cr_network_list_item/cr_network_list_item.js:68: * @default false On 2015/04/27 22:29:03, Jeremy Klein wrote: > nit: Isn't true the much more common case? I feel like this is a bit confusing > to have an "isListItem" property on a CrNetworkListItem element. It would be > more clear if this was just isStandAlone instead. Yeah, I kind of agree, but it matches cr-network-icon.isListItem, so I'd rather keep it as is.
Patchset #8 (id:140001) has been deleted
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org, jlklein@chromium.org Link to the patchset: https://codereview.chromium.org/1100993002/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1100993002/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9b80af9d3cc30689b982b5d627ca0f58a1dd473a Cr-Commit-Position: refs/heads/master@{#327224} |
