|
|
Created:
4 years, 2 months ago by dpapad Modified:
4 years, 2 months ago Reviewers:
Dan Beam CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Migrating add/edit credit card dropdowns to native select.
BUG=651513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/fa522478814cd467fd68c534d2e07c99cbd75be8
Cr-Commit-Position: refs/heads/master@{#422594}
Patch Set 1 : Resolve conflicts #Patch Set 2 : Fixing test. #Patch Set 3 : Nit. #
Total comments: 4
Patch Set 4 : Rebase #
Total comments: 6
Patch Set 5 : Address comments. #
Dependent Patchsets: Messages
Total messages: 26 (18 generated)
Description was changed from ========== MD Settings: Migrating add/edit credit card to native select. BUG=651513 ========== to ========== MD Settings: Migrating add/edit credit card to native select. BUG=651513 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== MD Settings: Migrating add/edit credit card to native select. BUG=651513 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating add/edit credit card dropdowns to native select. BUG=651513 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dpapad@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 dpapad@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 #1 (id:20001) has been deleted
The CQ bit was checked by dpapad@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...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
See before/after screenshots at http://imgur.com/a/gs4ML. Tests are on patch2 (expecting all green).
On 2016/09/29 at 23:24:57, dpapad wrote: > See before/after screenshots at http://imgur.com/a/gs4ML. Tests are on patch2 (expecting all green). Friendly ping.
https://codereview.chromium.org/2383513003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2383513003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:10: <style include="settings-shared select"> might be useful to eventually rename this to "md-select" or something slightly more specific (I don't like settings-select, mayyyyyyybe cr-select, but md-select sounds the best to me) https://codereview.chromium.org/2383513003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:23: #monthSelect { why "Select" in the ID? why not just #month? https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:7: <link rel="import" href="/select_css.html"> i think this should be md_select https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (left): https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:77: } revert this https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:88: Polymer.dom.flush(); i'm confused by this. could this be solved by moving this code (that runs on attached) into an observer of expirationYear_ and expirationMonth_, requiring them both to be set?
Patchset #5 (id:120001) has been deleted
Addressed comments. PTAL https://codereview.chromium.org/2383513003/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2383513003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:10: <style include="settings-shared select"> On 2016/10/03 at 18:24:48, Dan Beam wrote: > might be useful to eventually rename this to "md-select" or something slightly more specific (I don't like settings-select, mayyyyyyybe cr-select, but md-select sounds the best to me) As discussed, will rename to md-select after pending CLs have landed (it is more convenient that way). https://codereview.chromium.org/2383513003/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:23: #monthSelect { On 2016/10/03 at 18:24:48, Dan Beam wrote: > why "Select" in the ID? why not just #month? Done. https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:7: <link rel="import" href="/select_css.html"> On 2016/10/03 at 18:24:48, Dan Beam wrote: > i think this should be md_select Per discussion, will address in follow up CL. https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (left): https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:77: } On 2016/10/03 at 18:24:48, Dan Beam wrote: > revert this Done. https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2383513003/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:88: Polymer.dom.flush(); On 2016/10/03 at 18:24:48, Dan Beam wrote: > i'm confused by this. > > could this be solved by moving this code (that runs on attached) into an observer of expirationYear_ and expirationMonth_, requiring them both to be set? Updated. As discussed, using async() seems better than calling Polymer.dom.flush(). Updated tests accordingly.
lgtm
The CQ bit was checked by dpapad@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 #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Migrating add/edit credit card dropdowns to native select. BUG=651513 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Migrating add/edit credit card dropdowns to native select. BUG=651513 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/fa522478814cd467fd68c534d2e07c99cbd75be8 Cr-Commit-Position: refs/heads/master@{#422594} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fa522478814cd467fd68c534d2e07c99cbd75be8 Cr-Commit-Position: refs/heads/master@{#422594} |