Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808173003:
[MD settings] resize iron list when template expands (Closed)
|
Created:
3 years, 8 months ago by dschuyler Modified:
3 years, 8 months ago Reviewers:
hcarmona 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, scottchen Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] resize iron list when template expands
This CL is a workaround for expanding a template holding an iron list
that is also grown from zero elements. To get both things to happen in
the same JS update a dom flush is performed and an event is sent for the
iron list.
BUG=709868
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2808173003
Cr-Commit-Position: refs/heads/master@{#463486}
Committed: https://chromium.googlesource.com/chromium/src/+/6314de69d54e1882222d7b1fc5bcc0756d5bdefc
Patch Set 1 #
Total comments: 8
Patch Set 2 : comment change #
Total comments: 2
Patch Set 3 : nits #Messages
Total messages: 28 (19 generated)
Description was changed from ========== [MD settings] resize iron list when template expands This CL is a workaround for expanding a template holding an iron list that is also grown from zero elements. To get both things to happen in the same JS update a dom flush is performed and an event is sent for the iron list. BUG=709868 ========== to ========== [MD settings] resize iron list when template expands This CL is a workaround for expanding a template holding an iron list that is also grown from zero elements. To get both things to happen in the same JS update a dom flush is performed and an event is sent for the iron list. BUG=709868 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@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...
dschuyler@chromium.org changed reviewers: + hcarmona@chromium.org
https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:131: Polymer.dom.flush(); Do you need both |flush| and iron-resize? https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:132: this.$$('#list').fire('iron-resize'); this.$.list.fire(...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:131: Polymer.dom.flush(); On 2017/04/10 22:35:07, hcarmona wrote: > Do you need both |flush| and iron-resize? Without the flush, the #list is not there to be found. Without the event the #list doesn't show up. That's the reasoning behind having both (unless there's another way to do it). https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:132: this.$$('#list').fire('iron-resize'); On 2017/04/10 22:35:07, hcarmona wrote: > this.$.list.fire(...) $ doesn't find the id, while $$ will find the id.
LGTM https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:126: // When adding a word to an _empty_ list, the template is expanded. This nit: "This" is on both lines. https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:131: Polymer.dom.flush(); On 2017/04/10 23:13:57, dschuyler wrote: > On 2017/04/10 22:35:07, hcarmona wrote: > > Do you need both |flush| and iron-resize? > > Without the flush, the #list is not there to be found. Without the event the > #list doesn't show up. That's the reasoning behind having both (unless there's > another way to do it). Sounds good. https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:132: this.$$('#list').fire('iron-resize'); On 2017/04/10 23:13:57, dschuyler wrote: > On 2017/04/10 22:35:07, hcarmona wrote: > > this.$.list.fire(...) > > $ doesn't find the id, while $$ will find the id. Sounds good, I missed that this is in a dom-if.
The CQ bit was checked by dschuyler@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 checked by dschuyler@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:20001) has been deleted
https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2808173003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:126: // When adding a word to an _empty_ list, the template is expanded. This On 2017/04/10 23:19:39, hcarmona wrote: > nit: "This" is on both lines. Done.
https://codereview.chromium.org/2808173003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2808173003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:132: this.$$('#list').fire('iron-resize'); nit: fire('iron-resize') -> notifyResize()
The CQ bit was checked by dschuyler@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/2808173003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2808173003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:132: this.$$('#list').fire('iron-resize'); On 2017/04/10 23:32:49, Dan Beam wrote: > nit: fire('iron-resize') -> notifyResize() 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 dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hcarmona@chromium.org Link to the patchset: https://codereview.chromium.org/2808173003/#ps60001 (title: "nits")
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": 60001, "attempt_start_ts": 1491873113262420,
"parent_rev": "aab941796e90b42557028feefef8365d493b8651", "commit_rev":
"6314de69d54e1882222d7b1fc5bcc0756d5bdefc"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] resize iron list when template expands This CL is a workaround for expanding a template holding an iron list that is also grown from zero elements. To get both things to happen in the same JS update a dom flush is performed and an event is sent for the iron list. BUG=709868 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] resize iron list when template expands This CL is a workaround for expanding a template holding an iron list that is also grown from zero elements. To get both things to happen in the same JS update a dom flush is performed and an event is sent for the iron list. BUG=709868 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2808173003 Cr-Commit-Position: refs/heads/master@{#463486} Committed: https://chromium.googlesource.com/chromium/src/+/6314de69d54e1882222d7b1fc5bc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6314de69d54e1882222d7b1fc5bc... |
