Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2431583005:
MD Settings: Bluetooth: Use settings-action-menu dialog (Closed)
|
Created:
4 years, 2 months ago by stevenjb Modified:
4 years, 2 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Bluetooth: Use settings-action-menu dialog
Use settings-action-menu dialog instead of iron-dropdown in
bluetooth page.
This also introduces a helper behavior to settings_action_menu.
BUG=639718
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7d40a5bcfefafa85f491f80b35af27c51d914de7
Cr-Commit-Position: refs/heads/master@{#426389}
Patch Set 1 #Patch Set 2 : Simplify #
Total comments: 13
Patch Set 3 : Rebase, fix remove, feedback #
Total comments: 4
Patch Set 4 : Nits #
Messages
Total messages: 16 (6 generated)
Description was changed from ========== MD Settings: Bluetooth: Use settings-action-menu dialog Use settings-action-menu dialog instead of iron-dropdown in bluetooth page. This also introduces a helper behavior to settings_action_menu. BUG=639718 ========== to ========== MD Settings: Bluetooth: Use settings-action-menu dialog Use settings-action-menu dialog instead of iron-dropdown in bluetooth page. This also introduces a helper behavior to settings_action_menu. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
The CL description mentions a helper behavior, which I don't see in this CL. https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html (right): https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:38: <button id="connect" hidden$="[[device.connected]]" Is there a good reason to have connect/disconnect as separate buttons? - they both invoke the same tap listener, - only one can be shown at any given time. Why not merging them, and just change the displayed text? <button id="connectDisconnect" on-tap="onConnectDisconnectTap_"> [[getButtonText_(device.connected)]] </button> https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:46: <button id="remove">$i18n{bluetoothRemove}</button> Needs class="dropdown-item" otherwise it is ignored by the settings-action-menu. Also, shouldn't this have a listener? https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js (right): https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:30: event.stopPropagation(); Nit: Is stopPropagation() necessary? https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:43: if (menu.open) Is this check necessary? The menu should always be open at the time this listener executes, since user has just click on a button inside the menu.
https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html (right): https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-item/paper-item.html"> Still needed?
https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js (right): https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:26: onActionTap_: function(event) { Nit (optional): I find the onActionTap/onMenuTap usage here reverse from what I expect. An action can be tapped once the menu has been opened, whereas here it implies that the "..." button was tapped to open the menu. I suggest following the convention I've been using in other usages of settings-action-menu onDotsTap: indicates that the '...' button was tapped. on<ActionName>Tap: Indicates that a specific action inside the menu was clicked, for example onEditTap, onDeleteTap etc.
PTAL https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html (right): https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/paper-item/paper-item.html"> On 2016/10/18 23:32:18, dpapad wrote: > Still needed? Done. https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:38: <button id="connect" hidden$="[[device.connected]]" On 2016/10/18 23:28:45, dpapad wrote: > Is there a good reason to have connect/disconnect as separate buttons? > - they both invoke the same tap listener, > - only one can be shown at any given time. > > Why not merging them, and just change the displayed text? > > <button id="connectDisconnect" on-tap="onConnectDisconnectTap_"> > [[getButtonText_(device.connected)]] > </button> This seemed simpler, but sure, that works too. https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.html:46: <button id="remove">$i18n{bluetoothRemove}</button> On 2016/10/18 23:28:45, dpapad wrote: > Needs class="dropdown-item" otherwise it is ignored by the settings-action-menu. > Also, shouldn't this have a listener? Thanks. Done. https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js (right): https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:26: onActionTap_: function(event) { On 2016/10/18 23:36:50, dpapad wrote: > Nit (optional): I find the onActionTap/onMenuTap usage here reverse from what I > expect. > > An action can be tapped once the menu has been opened, whereas here it implies > that the "..." button was tapped to open the menu. I suggest following the > convention I've been using in other usages of settings-action-menu > > onDotsTap: indicates that the '...' button was tapped. > on<ActionName>Tap: Indicates that a specific action inside the menu was clicked, > for example onEditTap, onDeleteTap etc. Um, that confuses me, I'll just rename them all more explicitly. https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:30: event.stopPropagation(); On 2016/10/18 23:28:45, dpapad wrote: > Nit: Is stopPropagation() necessary? Yes. Clicking the parent has an effect. https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:43: if (menu.open) On 2016/10/18 23:28:45, dpapad wrote: > Is this check necessary? The menu should always be open at the time this > listener executes, since user has just click on a button inside the menu. It should be. I was just tying to avoid potential edge cases, I've noticed that we throw an error when calling close() on a non open dialog.
LGTM https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js (right): https://codereview.chromium.org/2431583005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:43: if (menu.open) On 2016/10/20 at 00:34:43, stevenjb wrote: > On 2016/10/18 23:28:45, dpapad wrote: > > Is this check necessary? The menu should always be open at the time this > > listener executes, since user has just click on a button inside the menu. > > It should be. I was just tying to avoid potential edge cases, I've noticed that we throw an error when calling close() on a non open dialog. Ok. Given that in this case it should always be open, if anything, I would prefer an assert() over silently swallowing the unexpected case where this code executes while the menu is closed. https://codereview.chromium.org/2431583005/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js (right): https://codereview.chromium.org/2431583005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:25: * @param {Event} event Nit: !Event https://codereview.chromium.org/2431583005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:61: return this.i18n(msg); Nit (optional): I think this might all fit in one line return this.i18n(connected ? 'bluetoothDisconnect' : 'bluetoothConnect');
https://codereview.chromium.org/2431583005/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js (right): https://codereview.chromium.org/2431583005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:25: * @param {Event} event On 2016/10/20 00:45:41, dpapad wrote: > Nit: !Event Done. https://codereview.chromium.org/2431583005/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/bluetooth_page/bluetooth_device_list_item.js:61: return this.i18n(msg); On 2016/10/20 00:45:41, dpapad wrote: > Nit (optional): I think this might all fit in one line > > return this.i18n(connected ? 'bluetoothDisconnect' : 'bluetoothConnect'); Done.
The CQ bit was checked by stevenjb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2431583005/#ps80001 (title: "Nits")
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 #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Bluetooth: Use settings-action-menu dialog Use settings-action-menu dialog instead of iron-dropdown in bluetooth page. This also introduces a helper behavior to settings_action_menu. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth: Use settings-action-menu dialog Use settings-action-menu dialog instead of iron-dropdown in bluetooth page. This also introduces a helper behavior to settings_action_menu. BUG=639718 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7d40a5bcfefafa85f491f80b35af27c51d914de7 Cr-Commit-Position: refs/heads/master@{#426389} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7d40a5bcfefafa85f491f80b35af27c51d914de7 Cr-Commit-Position: refs/heads/master@{#426389} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
