Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2304673002:
[CUPS] Implelment Manufacturer and Model Dialog. (Closed)
|
Created:
4 years, 3 months ago by xdai1 Modified:
4 years, 3 months ago Reviewers:
michaelpg 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. |
Description[CUPS] Implelment Manufacturer and Model Dialog.
- Also add printer Ip address validation in Manually-add-new-printer Dialog.
Note the APIs are not ready yet. So these are just UI dialogs and not
functional.
BUG=626752
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/fde6fc0b262e88a4f7d934d7fb39a9628a29b86d
Cr-Commit-Position: refs/heads/master@{#416803}
Patch Set 1 : [CUPS] Implelment Manufacturer and Model Dialog. #Patch Set 2 : Fix getConfiguringPrinterName_(). #
Total comments: 12
Patch Set 3 : Address michaelpg@'s comments. #
Total comments: 12
Patch Set 4 : Address michealpg@'s comments. #Patch Set 5 : fix the indent #Patch Set 6 : Address michaelpg@'s comment. Rebase. #
Messages
Total messages: 34 (22 generated)
Description was changed from ========== [CUPS] Implelment Manufacturer and Model Dialog. - Also add printer Ip address validation in Manually-add-new-printer Dialog. Note the APIs are not ready yet. So these are just UI dialogs and not functional. BUG=626752 ========== to ========== [CUPS] Implelment Manufacturer and Model Dialog. - Also add printer Ip address validation in Manually-add-new-printer Dialog. Note the APIs are not ready yet. So these are just UI dialogs and not functional. BUG=626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Patchset #2 (id:20001) 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...
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
xdai@chromium.org changed reviewers: + michaelpg@chromium.org
michaelpg@, could you help review this CL please? Thanks! You could see demo about how it looks like here: https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ
The CQ bit was checked by xdai@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...)
https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/paper-dropdown-menu/paper-dropdown-menu.html"> as a follow-up, please switch this file to use <paper-dropdown-menu-light>. settings shouldn't use <paper-dropdown-menu> anymore. note that the light version may require slightly different styling: https://github.com/PolymerElements/paper-dropdown-menu https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:119: <paper-input no-label-float id="printerAddressInput" import paper-input https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:166: <paper-spinner active></paper-spinner> import paper-spinner https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:122: this.$.searchInProgress.hidden = true; searchInProgress gets hidden in both cases? https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:37: <dom-module id="drop-down-search-box"> can i see some screenshots of this? https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:67: filter="[[filterItems_(searchTerm_)]]"> indent off
michaelpg@, I've addressed your comments. Please take another look, thanks! https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:3: <link rel="import" href="chrome://resources/polymer/v1_0/paper-dropdown-menu/paper-dropdown-menu.html"> On 2016/09/02 19:49:11, michaelpg wrote: > as a follow-up, please switch this file to use <paper-dropdown-menu-light>. > settings shouldn't use <paper-dropdown-menu> anymore. > > note that the light version may require slightly different styling: > https://github.com/PolymerElements/paper-dropdown-menu Done. https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:119: <paper-input no-label-float id="printerAddressInput" On 2016/09/02 19:49:11, michaelpg wrote: > import paper-input Done. https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:166: <paper-spinner active></paper-spinner> On 2016/09/02 19:49:11, michaelpg wrote: > import paper-spinner Done. https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:122: this.$.searchInProgress.hidden = true; On 2016/09/02 19:49:12, michaelpg wrote: > searchInProgress gets hidden in both cases? Right. We only show the searchInProgress spinner when we're in the searching process, and after we get the search result (found or not found the printer), we hide the spinner and show the found/not found message. Actually currently in the demo you could not see the transition from the spinner to the message because it's too quick. But after the API to check if it's a valid network printer's address is ready, the spinner will show up until the callback returns. https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:37: <dom-module id="drop-down-search-box"> On 2016/09/02 19:49:12, michaelpg wrote: > can i see some screenshots of this? Sure. I recorded a short video about the drop-down-search-box, please see https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ. https://codereview.chromium.org/2304673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:67: filter="[[filterItems_(searchTerm_)]]"> On 2016/09/02 19:49:12, michaelpg wrote: > indent off Done.
thanks for the video, looks awesome! we'll probably want to move the autocomplete dropdown to make it reusable at some point https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:41: iron-dropdown { nit: alphabetize this maybe https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:56: <paper-input no-label-float on-tap="onTap_" on-input="onInputValueChanged_" could be a performance issue with re-running the filter every keystroke. setting `type="search"` would let you use on-search instead of on-input. additionally, adding the `incremental` attribute will coalesce multiple keystrokes, so the filter updates after a timeout instead of immediately on keystroke: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-incremental https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:64: vertical-offset="47"> not a big fan of arbitrary numbers. will this work when scaling the page? maybe it would make sense to set this programatically when needed, using the paper-input offsetHeight?
michaelpg@, I've addressed your comments. Please take another look, thanks! https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:41: iron-dropdown { On 2016/09/02 21:51:55, michaelpg wrote: > nit: alphabetize this maybe Done. https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:56: <paper-input no-label-float on-tap="onTap_" on-input="onInputValueChanged_" On 2016/09/02 21:51:55, michaelpg wrote: > could be a performance issue with re-running the filter every keystroke. setting > `type="search"` would let you use on-search instead of on-input. additionally, > adding the `incremental` attribute will coalesce multiple keystrokes, so the > filter updates after a timeout instead of immediately on keystroke: > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-incremental It seems 'paper-input' doesn't have on-search event. Thus use 'input' here. https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:64: vertical-offset="47"> On 2016/09/02 21:51:55, michaelpg wrote: > not a big fan of arbitrary numbers. will this work when scaling the page? maybe > it would make sense to set this programatically when needed, using the > paper-input offsetHeight? It does work with scaling the page. I addressed your comment to use the offsetHeight instead, however, the dropdown window could not align with the bottom of the input very well. Could you take a look at the screenshot in https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ to see if it looks good to you?
The CQ bit was checked by xdai@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: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:56: <paper-input no-label-float on-tap="onTap_" on-input="onInputValueChanged_" On 2016/09/02 23:39:13, xdai1 wrote: > On 2016/09/02 21:51:55, michaelpg wrote: > > could be a performance issue with re-running the filter every keystroke. > setting > > `type="search"` would let you use on-search instead of on-input. additionally, > > adding the `incremental` attribute will coalesce multiple keystrokes, so the > > filter updates after a timeout instead of immediately on keystroke: > > > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-incremental > > It seems 'paper-input' doesn't have on-search event. Thus use 'input' here. Acknowledged. https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:64: vertical-offset="47"> On 2016/09/02 23:39:13, xdai1 wrote: > On 2016/09/02 21:51:55, michaelpg wrote: > > not a big fan of arbitrary numbers. will this work when scaling the page? > maybe > > it would make sense to set this programatically when needed, using the > > paper-input offsetHeight? > > It does work with scaling the page. I addressed your comment to use the > offsetHeight instead, however, the dropdown window could not align with the > bottom of the input very well. Could you take a look at the screenshot in > https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ to > see if it looks good to you? yeah, that looks weird. If you couldn't find a property with the right size to use, like offsetHeight, the constant here is fine.
https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:56: <paper-input no-label-float on-tap="onTap_" on-input="onInputValueChanged_" On 2016/09/06 22:24:40, michaelpg wrote: > On 2016/09/02 23:39:13, xdai1 wrote: > > On 2016/09/02 21:51:55, michaelpg wrote: > > > could be a performance issue with re-running the filter every keystroke. > > setting > > > `type="search"` would let you use on-search instead of on-input. > additionally, > > > adding the `incremental` attribute will coalesce multiple keystrokes, so the > > > filter updates after a timeout instead of immediately on keystroke: > > > > > > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-incremental > > > > It seems 'paper-input' doesn't have on-search event. Thus use 'input' here. > > Acknowledged. it does if you do this: https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search...
https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:56: <paper-input no-label-float on-tap="onTap_" on-input="onInputValueChanged_" On 2016/09/06 22:37:12, Dan Beam wrote: > On 2016/09/06 22:24:40, michaelpg wrote: > > On 2016/09/02 23:39:13, xdai1 wrote: > > > On 2016/09/02 21:51:55, michaelpg wrote: > > > > could be a performance issue with re-running the filter every keystroke. > > > setting > > > > `type="search"` would let you use on-search instead of on-input. > > additionally, > > > > adding the `incremental` attribute will coalesce multiple keystrokes, so > the > > > > filter updates after a timeout instead of immediately on keystroke: > > > > > > > > > > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-incremental > > > > > > It seems 'paper-input' doesn't have on-search event. Thus use 'input' here. > > > > Acknowledged. > > it does if you do this: > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... hmm, the difference between that and xdai1's latest patch (which you may not have seen yet) is that cr-search-field has the on-search event on the paper-input-container instead of the iron-input. I don't see the search event in paper-input-container so my guess is the cr-search-field example just grabs the bubbled event created by the input, so I'd argue xdai1's version is more correct.
michaelpg@, I've addressed your comments. Thanks for your review! https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html (right): https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:56: <paper-input no-label-float on-tap="onTap_" on-input="onInputValueChanged_" On 2016/09/06 22:58:52, michaelpg wrote: > On 2016/09/06 22:37:12, Dan Beam wrote: > > On 2016/09/06 22:24:40, michaelpg wrote: > > > On 2016/09/02 23:39:13, xdai1 wrote: > > > > On 2016/09/02 21:51:55, michaelpg wrote: > > > > > could be a performance issue with re-running the filter every keystroke. > > > > setting > > > > > `type="search"` would let you use on-search instead of on-input. > > > additionally, > > > > > adding the `incremental` attribute will coalesce multiple keystrokes, so > > the > > > > > filter updates after a timeout instead of immediately on keystroke: > > > > > > > > > > > > > > > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input#attr-incremental > > > > > > > > It seems 'paper-input' doesn't have on-search event. Thus use 'input' > here. > > > > > > Acknowledged. > > > > it does if you do this: > > > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_search... > > hmm, the difference between that and xdai1's latest patch (which you may not > have seen yet) is that cr-search-field has the on-search event on the > paper-input-container instead of the iron-input. I don't see the search event in > paper-input-container so my guess is the cr-search-field example just grabs the > bubbled event created by the input, so I'd argue xdai1's version is more > correct. Thanks for the explanation. https://codereview.chromium.org/2304673002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog_util.html:64: vertical-offset="47"> On 2016/09/06 22:24:40, michaelpg wrote: > On 2016/09/02 23:39:13, xdai1 wrote: > > On 2016/09/02 21:51:55, michaelpg wrote: > > > not a big fan of arbitrary numbers. will this work when scaling the page? > > maybe > > > it would make sense to set this programatically when needed, using the > > > paper-input offsetHeight? > > > > It does work with scaling the page. I addressed your comment to use the > > offsetHeight instead, however, the dropdown window could not align with the > > bottom of the input very well. Could you take a look at the screenshot in > > https://drive.google.com/corp/drive/u/0/folders/0B4R9nvLdzmC9d3o2WDNyNGRjejQ > to > > see if it looks good to you? > > yeah, that looks weird. If you couldn't find a property with the right size to > use, like offsetHeight, the constant here is fine. I don't find a proper way to dynamically determine the size. I reverted to use the constant here but will come back to it if I know a good way to do it in the future.
The CQ bit was checked by xdai@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: This issue passed the CQ dry run.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2304673002/#ps160001 (title: "Address michaelpg@'s comment. Rebase.")
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 #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [CUPS] Implelment Manufacturer and Model Dialog. - Also add printer Ip address validation in Manually-add-new-printer Dialog. Note the APIs are not ready yet. So these are just UI dialogs and not functional. BUG=626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Implelment Manufacturer and Model Dialog. - Also add printer Ip address validation in Manually-add-new-printer Dialog. Note the APIs are not ready yet. So these are just UI dialogs and not functional. BUG=626752 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/fde6fc0b262e88a4f7d934d7fb39a9628a29b86d Cr-Commit-Position: refs/heads/master@{#416803} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/fde6fc0b262e88a4f7d934d7fb39a9628a29b86d Cr-Commit-Position: refs/heads/master@{#416803} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
