Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2733963002:
MD Settings: Debounce navigation event in side nav. (Closed)
|
Created:
3 years, 9 months ago by hcarmona Modified:
3 years, 9 months ago 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Debounce navigation event in side nav.
Side nav was causing 2 navigation events because |setSelectedUrl_| sets
the selected URL on both selectors. Debounce will make sure only one of
these events is handled. Timeout is not necessary.
R=dschuyler@gmail.com
BUG=696458
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2733963002
Cr-Commit-Position: refs/heads/master@{#457475}
Committed: https://chromium.googlesource.com/chromium/src/+/fc3c4c428da2efaacc676dc40a1f2973daadc71d
Patch Set 1 #Patch Set 2 : fix test #
Total comments: 5
Patch Set 3 : nit #Patch Set 4 : feedback #Messages
Total messages: 31 (23 generated)
Description was changed from ========== MD Settings: Debounce navigation event in side nav. Side nav was causing 2 navigation events because |setSelectedUrl_| sets the selected URL on both selectors. Debounce will make sure only one of these events is handled. Timeout is not necessary. R=dschuyler@gmail.com BUG=696458 ========== to ========== MD Settings: Debounce navigation event in side nav. Side nav was causing 2 navigation events because |setSelectedUrl_| sets the selected URL on both selectors. Debounce will make sure only one of these events is handled. Timeout is not necessary. R=dschuyler@gmail.com BUG=696458 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
Description was changed from ========== MD Settings: Debounce navigation event in side nav. Side nav was causing 2 navigation events because |setSelectedUrl_| sets the selected URL on both selectors. Debounce will make sure only one of these events is handled. Timeout is not necessary. R=dschuyler@gmail.com BUG=696458 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Debounce navigation event in side nav. Side nav was causing 2 navigation events because |setSelectedUrl_| sets the selected URL on both selectors. Debounce will make sure only one of these events is handled. Timeout is not necessary. R=dschuyler@gmail.com BUG=696458 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dbeam@chromium.org changed reviewers: + dschuyler@chromium.org - dschuyler@gmail.com
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2733963002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2733963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:74: onSelectorActivate_: function(event) { I think we can remove the inner (nested) onSelectorActivate_ in the html file -- without adding the debounce. Does that work for you, WDYT? https://codereview.chromium.org/2733963002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_menu_test.js (right): https://codereview.chromium.org/2733963002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_menu_test.js:75: }, 0); nit: zero is the default msec value (so it's not needed).
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2733963002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2733963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:74: onSelectorActivate_: function(event) { On 2017/03/15 01:13:02, dschuyler wrote: > I think we can remove the inner (nested) onSelectorActivate_ in the html file -- > without adding the debounce. Does that work for you, WDYT? This unfortunately doesn't work because the items in the advanced section aren't direct children of the top iron-selector. :-\ The reason we get a double notification is because we set the other selector in setSelectedUrl_, the HTML isn't double notifying https://codereview.chromium.org/2733963002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_menu_test.js (right): https://codereview.chromium.org/2733963002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_menu_test.js:75: }, 0); On 2017/03/15 01:13:02, dschuyler wrote: > nit: zero is the default msec value (so it's not needed). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Dry run: 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 hcarmona@chromium.org
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2733963002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/settings_menu/settings_menu.js (right): https://codereview.chromium.org/2733963002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/settings_menu/settings_menu.js:74: onSelectorActivate_: function(event) { On 2017/03/15 19:22:09, hcarmona wrote: > On 2017/03/15 01:13:02, dschuyler wrote: > > I think we can remove the inner (nested) onSelectorActivate_ in the html file > -- > > without adding the debounce. Does that work for you, WDYT? > > This unfortunately doesn't work because the items in the advanced section aren't > direct children of the top iron-selector. :-\ > > The reason we get a double notification is because we set the other selector in > setSelectedUrl_, the HTML isn't double notifying We spoke in person and I had misunderstood what you meant. Your proposed solution works perfectly. This is because Polymer doesn't do filtering on the target, so when an element listens it'll capture its children's events also. I also said we might be able to get rid of some other properties: we need them in order to highlight properly.
lgtm
The CQ bit was checked by hcarmona@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489681647197160,
"parent_rev": "a136ffb89abdc69dfbebdf1aeb3a1c7ab28d2902", "commit_rev":
"fc3c4c428da2efaacc676dc40a1f2973daadc71d"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Debounce navigation event in side nav. Side nav was causing 2 navigation events because |setSelectedUrl_| sets the selected URL on both selectors. Debounce will make sure only one of these events is handled. Timeout is not necessary. R=dschuyler@gmail.com BUG=696458 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Debounce navigation event in side nav. Side nav was causing 2 navigation events because |setSelectedUrl_| sets the selected URL on both selectors. Debounce will make sure only one of these events is handled. Timeout is not necessary. R=dschuyler@gmail.com BUG=696458 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2733963002 Cr-Commit-Position: refs/heads/master@{#457475} Committed: https://chromium.googlesource.com/chromium/src/+/fc3c4c428da2efaacc676dc40a1f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/fc3c4c428da2efaacc676dc40a1f... |
