Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2300753004:
MD Settings menu should be visible (Closed)
|
Created:
4 years, 3 months ago by michaelpg Modified:
4 years, 3 months ago Reviewers:
Dan Beam CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings menu should be visible
Currently the settings-menu is lazy-created when the app-drawer is opened.
But the app-drawer can be visible even when its opened property is false, like
when the drawer is being dragged partway open, or while it is closing. In these
cases, the settings-menu should still be stamped.
BUG=642694
R=dbeam@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/64b7d7d026f7ef0d69c59845cc20b5649c48c8c7
Cr-Commit-Position: refs/heads/master@{#416846}
Patch Set 1 #Patch Set 2 : cleanup #
Total comments: 14
Patch Set 3 : feedback #
Total comments: 4
Patch Set 4 : dbeam feedback #Patch Set 5 : closure #
Messages
Total messages: 28 (14 generated)
Description was changed from ========== MD Settings menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dschuyler@chromium.org ========== to ========== MD Settings menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dschuyler@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
michaelpg@chromium.org changed reviewers: + dbeam@chromium.org - dschuyler@chromium.org
WDYT? https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:405: target.addEventListener(eventName, function runOnce(event) { dbeam: your example returned a Promise, but this way is more general-purpose (and can be used synchronously when necessary) https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:406: target.removeEventListener(eventName, runOnce, false); apparently closure requires this 3rd argument for EventTarget...
https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:89: <app-drawer swipe-open opened="{{drawerOpened_}}"> did you mean to remove this reference to "drawerOpened_" here? https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:54: listenOnce(drawer, 'track', this.onDrawerFirstShown_.bind(this)); don't we really want this to fire once only on the first event, not on the first of *each* event? https://codereview.chromium.org/2300753004/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_ui_browsertest.js (right): https://codereview.chromium.org/2300753004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_ui_browsertest.js:50: drawer.fire('track', { state: 'start' }); {\s -> { https://codereview.chromium.org/2300753004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_ui_browsertest.js:50: drawer.fire('track', { state: 'start' }); \s} -> } https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:400: * @param {string} eventName nit: maybe support multiple events? ... Callback is called and listening stops on the first event triggered. ... @param {!Array<!string>|!string} eventNames An array of event types or a space-separated list as a string (i.e. 'click mousedown'). var eventNames = Array.isArray(eventNames) ? eventNames : eventNames.split(/\s+/); function removeAllAndCallback() { eventNames.forEach(function(eventName) { target.removeEventListener(eventName, removeAllAndCallback); }); return callback(event); } eventNames.forEach(function(eventName) { target.addEventListener(eventName, removeAllAndCallback); }); listenOnce(window, 'scroll resize', reposition); https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:408: }, false); we'll probably need some type @param {boolean=} opt_capture eventually, fine to wait on that if we'd like
Addressed comments. Thanks! https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.html (right): https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.html:89: <app-drawer swipe-open opened="{{drawerOpened_}}"> On 2016/09/01 01:33:12, Dan Beam wrote: > did you mean to remove this reference to "drawerOpened_" here? Done. https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2300753004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:54: listenOnce(drawer, 'track', this.onDrawerFirstShown_.bind(this)); On 2016/09/01 01:33:13, Dan Beam wrote: > don't we really want this to fire once only on the first event, not on the first > of *each* event? Yeah. It wasn't a big deal since it's basically a no-op the second time but this is more correcter. https://codereview.chromium.org/2300753004/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_ui_browsertest.js (right): https://codereview.chromium.org/2300753004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_ui_browsertest.js:50: drawer.fire('track', { state: 'start' }); On 2016/09/01 01:33:13, Dan Beam wrote: > {\s -> { Dones. https://codereview.chromium.org/2300753004/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_ui_browsertest.js:50: drawer.fire('track', { state: 'start' }); On 2016/09/01 01:33:13, Dan Beam wrote: > \s} -> } Dones. https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:400: * @param {string} eventName On 2016/09/01 01:33:13, Dan Beam wrote: > nit: maybe support multiple events? > > ... Callback is called and listening stops on the first event triggered. ... > > @param {!Array<!string>|!string} eventNames An array of event types or a > space-separated list as a string (i.e. 'click mousedown'). > > var eventNames = > Array.isArray(eventNames) ? eventNames : eventNames.split(/\s+/); > > function removeAllAndCallback() { > eventNames.forEach(function(eventName) { > target.removeEventListener(eventName, removeAllAndCallback); > }); > return callback(event); > } > > eventNames.forEach(function(eventName) { > target.addEventListener(eventName, removeAllAndCallback); > }); > > listenOnce(window, 'scroll resize', reposition); Done. I had briefly considered this but thought it may be overkill. https://codereview.chromium.org/2300753004/diff/20001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:408: }, false); On 2016/09/01 01:33:13, Dan Beam wrote: > we'll probably need some type @param {boolean=} opt_capture eventually, fine to > wait on that if we'd like Acknowledged (does anyone actually use that? I've forgotten how it works more often than I've used it.)
too bad we can't just bind to <app-drawer>#_drawerState, that'd probably be simpler lgtm https://codereview.chromium.org/2300753004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2300753004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:54: drawer, 'track opened-changed', this.onDrawerFirstShown_.bind(this)); nit: listenOnce(this.$$('app-drawer'), 'track opened-changed', function() { this.$.drawerTemplate.if = true; }.bind(this)); https://codereview.chromium.org/2300753004/diff/40001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2300753004/diff/40001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:410: for (var eventName of eventNames) this code runs on iOS, where for..of support may be dodgy (that's why I didn't use in my example) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/... http://kangax.github.io/compat-table/es6/#test-for..of_loops enjoy your fun choices!
https://codereview.chromium.org/2300753004/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2300753004/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/settings_ui/settings_ui.js:54: drawer, 'track opened-changed', this.onDrawerFirstShown_.bind(this)); On 2016/09/01 03:02:01, Dan Beam wrote: > nit: > > listenOnce(this.$$('app-drawer'), 'track opened-changed', function() { > this.$.drawerTemplate.if = true; > }.bind(this)); Done. https://codereview.chromium.org/2300753004/diff/40001/ui/webui/resources/js/u... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/2300753004/diff/40001/ui/webui/resources/js/u... ui/webui/resources/js/util.js:410: for (var eventName of eventNames) On 2016/09/01 03:02:01, Dan Beam wrote: > this code runs on iOS, where for..of support may be dodgy (that's why I didn't > use in my example) > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/... > http://kangax.github.io/compat-table/es6/#test-for..of_loops > > enjoy your fun choices! Done.
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2300753004/#ps60001 (title: "dbeam feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2300753004/#ps80001 (title: "closure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by michaelpg@chromium.org
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.
Description was changed from ========== MD Settings menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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 menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings menu should be visible Currently the settings-menu is lazy-created when the app-drawer is opened. But the app-drawer can be visible even when its opened property is false, like when the drawer is being dragged partway open, or while it is closing. In these cases, the settings-menu should still be stamped. BUG=642694 R=dbeam@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/64b7d7d026f7ef0d69c59845cc20b5649c48c8c7 Cr-Commit-Position: refs/heads/master@{#416846} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/64b7d7d026f7ef0d69c59845cc20b5649c48c8c7 Cr-Commit-Position: refs/heads/master@{#416846} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
