Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2370843002:
Site Settings Desktop: Only show Remove action if filter hasn't removed all. (Closed)
|
Created:
4 years, 2 months ago by Finnur Modified:
4 years, 2 months ago Reviewers:
tommycli 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. |
DescriptionSite Settings Desktop: Only show Remove action if filter hasn't removed all.
BUG=650239
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/c1dfa6953598067e94cf4afec495f727c3924000
Cr-Commit-Position: refs/heads/master@{#421275}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address feedback #
Messages
Total messages: 33 (25 generated)
Description was changed from ========== Site Settings Desktop: Only show Remove action if filter hasn't removed all. BUG=650239 ========== to ========== Site Settings Desktop: Only show Remove action if filter hasn't removed all. BUG=650239 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by finnur@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...
finnur@chromium.org changed reviewers: + tommycli@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 finnur@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2370843002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data.js (right): https://codereview.chromium.org/2370843002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data.js:78: // There seems to be a bug in renderedItemCount where it is always 0 after why move this code from the computed binding? Wouldn't this break now when this.sites is updated? https://codereview.chromium.org/2370843002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data.js:84: this.$.remove.hidden = this.$.list['renderedItemCount'] == 0; list.renderedItemCount?
https://codereview.chromium.org/2370843002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data.js (right): https://codereview.chromium.org/2370843002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data.js:84: this.$.remove.hidden = this.$.list['renderedItemCount'] == 0; On 2016/09/26 23:01:34, tommycli wrote: > list.renderedItemCount? this is why: https://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compil... finnur@: I think you just need to cast to /** @type {DomRepeatElement} */(this.$.list) and I need to add that property to Polymer's externs: https://github.com/google/closure-compiler/blob/master/contrib/externs/polyme...
The CQ bit was checked by finnur@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: 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 finnur@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...
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by finnur@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/2370843002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_data.js (right): https://codereview.chromium.org/2370843002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data.js:78: // There seems to be a bug in renderedItemCount where it is always 0 after > why move this code from the computed binding? Good question. I tried both ways before and this is the one that worked. Your suggestion, however, prompted me to try the other way again and this time it not only worked but also means less code, avoids the bug mentioned in the comment and there's even no need to update the externs. Win win win situation. :) Thanks for suggesting! :) https://codereview.chromium.org/2370843002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_data.js:84: this.$.remove.hidden = this.$.list['renderedItemCount'] == 0; Yup. Dan hit the nail on the head, but it is all moot now anyway.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm thanks!
The CQ bit was checked by finnur@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.
Committed patchset #2 (id:2)
Message was sent while issue was closed.
Description was changed from ========== Site Settings Desktop: Only show Remove action if filter hasn't removed all. BUG=650239 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Only show Remove action if filter hasn't removed all. BUG=650239 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/c1dfa6953598067e94cf4afec495f727c3924000 Cr-Commit-Position: refs/heads/master@{#421275} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c1dfa6953598067e94cf4afec495f727c3924000 Cr-Commit-Position: refs/heads/master@{#421275} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
