Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453773003:
Alerting the user when the provided date for credit card is passed, not save the card, and not clos… (Closed)
DescriptionAlerting the user when the provided date for credit card is passed, not save the card, and not close the dialog.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/86dcf032ed6a80f0c68f13b6cb7ec0c66a26ec9e
Cr-Commit-Position: refs/heads/master@{#429931}
Patch Set 1 #
Total comments: 34
Patch Set 2 : Alerting the user when the entered CC exp. date is passed, and not saving it. #
Total comments: 13
Patch Set 3 : Correcting minor style errors. #
Total comments: 2
Patch Set 4 : Make the UI look like mocks again. #
Messages
Total messages: 36 (20 generated)
Description was changed from ========== Alerting the user when the provided date for credit card is passed, not save the card, and not close the dialog. BUG= ========== to ========== Alerting the user when the provided date for credit card is passed, not save the card, and not close the dialog. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
parastoog@google.com changed reviewers: + mahmadi@google.com, mathp@google.com
parastoog@google.com changed reviewers: + madi@chromium.org, mathp@chromium.org - mathp@google.com
parastoog@google.com changed reviewers: + mahmadi@chromium.org - madi@chromium.org, mahmadi@google.com
Thanks Parastoo! Some comments https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:345: nit: extra new line https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:61: <span style="color:red;"> this should probably be a class in the above <style> block https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:84: nit: remove extra line https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:124: if (this.expirationYear_ <= this.currentYear_ || Add a comment above this block: // If the card is expired, see the appropriate property which will reflect the error to the user. Otherwise, update the card, save and close the dialog. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:139: if (this.expirationMonth_ >= this.currentMonth_) { if (this.dateExpired_ && this.expirationMonth_.... https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:150: nit: remove extra new line https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:311: creditCard.expirationMonth = now.getMonth() - 1; If this test runs in January this will be -1? https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:326: window.setTimeout(done, 100); is it possible to assert that the error shows?
parastoog@google.com changed reviewers: - mahmadi@chromium.org
https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:345: On 2016/10/27 15:49:56, Mathieu Perreault wrote: > nit: extra new line Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:61: <span style="color:red;"> On 2016/10/27 15:49:56, Mathieu Perreault wrote: > this should probably be a class in the above <style> block Acknowledged. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:84: On 2016/10/27 15:49:56, Mathieu Perreault wrote: > nit: remove extra line Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:124: if (this.expirationYear_ <= this.currentYear_ || On 2016/10/27 15:49:56, Mathieu Perreault wrote: > Add a comment above this block: > > // If the card is expired, see the appropriate property which will reflect the > error to the user. Otherwise, update the card, save and close the dialog. Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:139: if (this.expirationMonth_ >= this.currentMonth_) { On 2016/10/27 15:49:56, Mathieu Perreault wrote: > if (this.dateExpired_ && this.expirationMonth_.... I think this is something the compiler will take care of: if it is false, do nothing. Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:150: On 2016/10/27 15:49:56, Mathieu Perreault wrote: > nit: remove extra new line Done. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:311: creditCard.expirationMonth = now.getMonth() - 1; On 2016/10/27 15:49:56, Mathieu Perreault wrote: > If this test runs in January this will be -1? Yes, and it's safe. I tested it. Is it OK to leave it as is? need a comment? https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:326: window.setTimeout(done, 100); On 2016/10/27 15:49:57, Mathieu Perreault wrote: > is it possible to assert that the error shows? I have to figure out how.
mahmadi@chromium.org changed reviewers: + mahmadi@chromium.org
Overall looks good. only a few minor changes and nits here and there. code review tip for more efficient reviews (from experience): when addressing reviewers' comments (e.g., when saying "done") submit your draft message after you upload the next patch because the next reviewers see the final state of your code and it usually gives you more useful feedback and faster lgtm :) https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:342: <message name="IDS_SETTINGS_CREDIT_CARD_EXPIRED" desc="Shown when an expired date is entered for credit card."> Try to be more accurate in desc. for example: "The error message that is shown when user attempts to save an expired credit card." https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:61: <span style="color:red;"> On 2016/10/27 16:08:40, parastoog wrote: > On 2016/10/27 15:49:56, Mathieu Perreault wrote: > > this should probably be a class in the above <style> block > > Acknowledged. Don't use hex or color names in Polymer. Use variables defined in src/third_party/polymer/v1_0/components-chromium/paper-styles/color.html e.g., color: var(--google-red-100) or whatever the mocks say. Also, I don't think you need to use dom-if template here for such a small block of markup. It comes with an overhead. use 'hidden' attr to hide/show the <span>. If you do, however, use dom-if wrap the entire <span> in it. why leaving an empty <span> in the markup if there's no error to show? and last, use one-way binding (https://www.polymer-project.org/1.0/docs/devguide/data-binding#binding-annota...) when the property is used in a read-only way. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:60: dateExpired_: Boolean, nit: cardExpired_ or something like that https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:139: if (this.expirationMonth_ >= this.currentMonth_) { On 2016/10/27 16:08:40, parastoog wrote: > On 2016/10/27 15:49:56, Mathieu Perreault wrote: > > if (this.dateExpired_ && this.expirationMonth_.... > > I think this is something the compiler will take care of: if it is false, do > nothing. > > Done. Wait, changing the month alone doesn't guarantee the card isn't expired. the year could still be in the past, right? I suggest adding a private helper function like isCardExpired_ that does the month and the year calculations and returns a boolean. Also, currentYear_ and currentMonth_ don't need to be declared properties. simply call Date.prototype.getFullYear() and Date.prototype.getMonth() in your helper function and use local variables if needed. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:310: creditCard.expirationYear = now.getFullYear(); Can you refactor your test to have a test for year too? both year and month affect the expiry. You should test both. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:321: done(); I don't think this last line will be even reached. the test should fail at the assert. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:326: window.setTimeout(done, 100); On 2016/10/27 16:08:40, parastoog wrote: > On 2016/10/27 15:49:57, Mathieu Perreault wrote: > > is it possible to assert that the error shows? > > I have to figure out how. assertTrue(!!creditCardDialog.$$(...)) if you're using dom-if and assertTrue(!!creditCardDialog.$$(...).clientHeight > 0) if you're using hidden attr should work.
https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:326: window.setTimeout(done, 100); On 2016/10/27 18:31:11, moe wrote: > On 2016/10/27 16:08:40, parastoog wrote: > > On 2016/10/27 15:49:57, Mathieu Perreault wrote: > > > is it possible to assert that the error shows? > > > > I have to figure out how. > > assertTrue(!!creditCardDialog.$$(...)) if you're using dom-if and > assertTrue(!!creditCardDialog.$$(...).clientHeight > 0) if you're using hidden > attr should work. correction: assertTrue(creditCardDialog.$$(...).clientHeight > 0) (no need for the bangs)
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2453773003/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:342: <message name="IDS_SETTINGS_CREDIT_CARD_EXPIRED" desc="Shown when an expired date is entered for credit card."> On 2016/10/27 18:31:11, moe wrote: > Try to be more accurate in desc. for example: "The error message that is shown > when user attempts to save an expired credit card." Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:61: <span style="color:red;"> On 2016/10/27 16:08:40, parastoog wrote: > On 2016/10/27 15:49:56, Mathieu Perreault wrote: > > this should probably be a class in the above <style> block > > Acknowledged. Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:61: <span style="color:red;"> On 2016/10/27 18:31:11, moe wrote: > On 2016/10/27 16:08:40, parastoog wrote: > > On 2016/10/27 15:49:56, Mathieu Perreault wrote: > > > this should probably be a class in the above <style> block > > > > Acknowledged. > > Don't use hex or color names in Polymer. Use variables defined in > src/third_party/polymer/v1_0/components-chromium/paper-styles/color.html > e.g., > color: var(--google-red-100) or whatever the mocks say. > > Also, I don't think you need to use dom-if template here for such a small block > of markup. It comes with an overhead. use 'hidden' attr to hide/show the <span>. > If you do, however, use dom-if wrap the entire <span> in it. why leaving an > empty <span> in the markup if there's no error to show? > > and last, use one-way binding > (https://www.polymer-project.org/1.0/docs/devguide/data-binding#binding-annota...) > when the property is used in a read-only way. Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:60: dateExpired_: Boolean, On 2016/10/27 18:31:11, moe wrote: > nit: cardExpired_ or something like that Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:139: if (this.expirationMonth_ >= this.currentMonth_) { On 2016/10/27 18:31:11, moe wrote: > On 2016/10/27 16:08:40, parastoog wrote: > > On 2016/10/27 15:49:56, Mathieu Perreault wrote: > > > if (this.dateExpired_ && this.expirationMonth_.... > > > > I think this is something the compiler will take care of: if it is false, do > > nothing. > > > > Done. > > Wait, changing the month alone doesn't guarantee the card isn't expired. the > year could still be in the past, right? I suggest adding a private helper > function like isCardExpired_ that does the month and the year calculations and > returns a boolean. > Also, currentYear_ and currentMonth_ don't need to be declared properties. > simply call Date.prototype.getFullYear() and Date.prototype.getMonth() in your > helper function and use local variables if needed. Done. https://codereview.chromium.org/2453773003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:139: if (this.expirationMonth_ >= this.currentMonth_) { On 2016/10/27 18:31:11, moe wrote: > On 2016/10/27 16:08:40, parastoog wrote: > > On 2016/10/27 15:49:56, Mathieu Perreault wrote: > > > if (this.dateExpired_ && this.expirationMonth_.... > > > > I think this is something the compiler will take care of: if it is false, do > > nothing. > > > > Done. > > Wait, changing the month alone doesn't guarantee the card isn't expired. the > year could still be in the past, right? I suggest adding a private helper > function like isCardExpired_ that does the month and the year calculations and > returns a boolean. > Also, currentYear_ and currentMonth_ don't need to be declared properties. > simply call Date.prototype.getFullYear() and Date.prototype.getMonth() in your > helper function and use local variables if needed. According to the year list --which doesn't show past years-- changing month alone does so, but anyway I have already added the helper function, which checks for both year and month. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:310: creditCard.expirationYear = now.getFullYear(); On 2016/10/27 18:31:11, moe wrote: > Can you refactor your test to have a test for year too? both year and month > affect the expiry. You should test both. Done. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:311: creditCard.expirationMonth = now.getMonth() - 1; On 2016/10/27 16:08:40, parastoog wrote: > On 2016/10/27 15:49:56, Mathieu Perreault wrote: > > If this test runs in January this will be -1? > > Yes, and it's safe. I tested it. Is it OK to leave it as is? need a comment? Done. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:321: done(); On 2016/10/27 18:31:11, moe wrote: > I don't think this last line will be even reached. the test should fail at the > assert. Done. https://codereview.chromium.org/2453773003/diff/1/chrome/test/data/webui/sett... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:326: window.setTimeout(done, 100); On 2016/10/27 18:35:08, moe wrote: > On 2016/10/27 18:31:11, moe wrote: > > On 2016/10/27 16:08:40, parastoog wrote: > > > On 2016/10/27 15:49:57, Mathieu Perreault wrote: > > > > is it possible to assert that the error shows? > > > > > > I have to figure out how. > > > > assertTrue(!!creditCardDialog.$$(...)) if you're using dom-if and > > assertTrue(!!creditCardDialog.$$(...).clientHeight > 0) if you're using hidden > > attr should work. > > correction: assertTrue(creditCardDialog.$$(...).clientHeight > 0) > (no need for the bangs) Done.
Thanks Parastoo. a few more nits ;) when making UI changes, it's always a good idea to send screenshots of your changes along. checkout go/snipit or feel free to use imgur.com or similar services. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:21: with: 70px; is 70px suggested by the mocks? https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:56: value: false nit: use trailing comma either everywhere or nowhere. try to be consistent with the file you're modifying. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:65: * Returns true iff the provided expiration date is passed. @return {boolean} True if ... side note: you can always run third_party/closure_compiler/run_compiler to see if your code "closure compiles" to avoid any surprises when trying to submit your code. another useful step is to run 'git-cl try' after you submit a patch to make sure all commit bots are green. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:71: (this.expirationYear_ == now.getFullYear() && indent this and the next line right by 4 to align with 'this' in the line above. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:72: this.expirationMonth_ <= now.getMonth())); is it really the case that you can't use a credit card on its last month of validity? also indent this line right by 1 to align with 'this' on the line above. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:128: // If the card is expired, see the appropriate property which will reflect s/see/set/
The CQ bit was checked by parastoog@google.com 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 if the issues are addressed https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html (right): https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:21: with: 70px; nit: width Also +1 Moe's suggestion of verifying with a UX person by sending them a screenshot (hwi@ is a good person to ping). Thanks!
parastoog@google.com changed reviewers: + tommycli@chromium.org
tommycli@chromium.org: Please review changes in https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:56: value: false On 2016/10/27 22:42:30, moe wrote: > nit: use trailing comma either everywhere or nowhere. try to be consistent with > the file you're modifying. Done. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:65: * Returns true iff the provided expiration date is passed. On 2016/10/27 22:42:30, moe wrote: > @return {boolean} True if ... > > side note: you can always run third_party/closure_compiler/run_compiler to see > if your code "closure compiles" to avoid any surprises when trying to submit > your code. another useful step is to run 'git-cl try' after you submit a patch > to make sure all commit bots are green. Done. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:71: (this.expirationYear_ == now.getFullYear() && On 2016/10/27 22:42:30, moe wrote: > indent this and the next line right by 4 to align with 'this' in the line above. Done. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:72: this.expirationMonth_ <= now.getMonth())); On 2016/10/27 22:42:30, moe wrote: > is it really the case that you can't use a credit card on its last month of > validity? > > also indent this line right by 1 to align with 'this' on the line above. You can use it in the last month, but getMonth return value starts from 0, and expirationMonth form 1. So, this is now the case where you can use it in the last month. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:72: this.expirationMonth_ <= now.getMonth())); On 2016/10/27 22:42:30, moe wrote: > is it really the case that you can't use a credit card on its last month of > validity? > > also indent this line right by 1 to align with 'this' on the line above. Done. https://codereview.chromium.org/2453773003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:128: // If the card is expired, see the appropriate property which will reflect On 2016/10/27 22:42:30, moe wrote: > s/see/set/ Done.
lgtm except: https://codereview.chromium.org/2453773003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:54: cardExpired_: { Since you already have a isCardExpired_ function, can you just use a computed binding instead of a keeping a separate property in sync: https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-comp...
https://codereview.chromium.org/2453773003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js (right): https://codereview.chromium.org/2453773003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:54: cardExpired_: { On 2016/10/28 22:30:54, tommycli wrote: > Since you already have a isCardExpired_ function, can you just use a computed > binding instead of a keeping a separate property in sync: > > https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-comp... Done.
The CQ bit was checked by parastoog@google.com 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 #4 (id:80001) has been deleted
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 parastoog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mahmadi@chromium.org, mathp@chromium.org, tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2453773003/#ps100001 (title: "Make the UI look like mocks again.")
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 #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Alerting the user when the provided date for credit card is passed, not save the card, and not close the dialog. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Alerting the user when the provided date for credit card is passed, not save the card, and not close the dialog. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/86dcf032ed6a80f0c68f13b6cb7ec0c66a26ec9e Cr-Commit-Position: refs/heads/master@{#429931} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/86dcf032ed6a80f0c68f13b6cb7ec0c66a26ec9e Cr-Commit-Position: refs/heads/master@{#429931} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
