Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557873002:
[Payments] Move focus to next field in expired cvc unmask prompt. (Closed)
|
Created:
4 years ago by sebsg Modified:
4 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Move focus to next field in expired cvc unmask prompt.
Moves the focus to the expiration year field when the expiration month
field gets filled with a second character and move the focus to the
cvc input field when the expiration year field gets filled with a
second character.
Also adds more detailed error messages for invalid expiration month and
year.
BUG=671762
Committed: https://crrev.com/2f32a659f1da7d65a343bff11bb8544969ecf276
Cr-Commit-Position: refs/heads/master@{#437571}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed Rouslan's comments #
Total comments: 6
Patch Set 3 : Used constant #
Messages
Total messages: 33 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focuse to the cvc input field when the expiration month field gets filled with a second character. BUG=671762 ========== to ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focuse to the cvc input field when the expiration month field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
The CQ bit was checked by sebsg@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...
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Math, PTAL?
Description was changed from ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focuse to the cvc input field when the expiration month field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 ========== to ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focuse to the cvc input field when the expiration month field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 ==========
Description was changed from ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focuse to the cvc input field when the expiration month field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 ========== to ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focus to the cvc input field when the expiration year field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 ==========
Do you have a video? https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:526: default: Default usually goes to the bottom and you don't need the two cases above it. https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:590: private int areExpirationAndCvcValid() { This does not return a boolean, so it's inadvisable to use "is" and "are" prefixes. "get" works better. For example, "getExpirationAndCvcErrorType()". https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:590: private int areExpirationAndCvcValid() { @ErrorType https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:594: if (mShouldRequestExpirationDate) { Single line? https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:624: */ @ErrorType private int getExpirationDateErrorType() { https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:771: * Returns -1 if the input is empty or otherwise not a valid month. You can't say "otherwise not a valid month", because this method can return 99, for example. Maybe you wanted to say "input is not a number".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Another look? https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:526: default: On 2016/12/08 18:59:59, rouslan wrote: > Default usually goes to the bottom and you don't need the two cases above it. I moved it down But I get warnings if I remove the two cases. https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:590: private int areExpirationAndCvcValid() { On 2016/12/08 18:59:58, rouslan wrote: > This does not return a boolean, so it's inadvisable to use "is" and "are" > prefixes. "get" works better. For example, "getExpirationAndCvcErrorType()". Done. https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:590: private int areExpirationAndCvcValid() { On 2016/12/08 18:59:58, rouslan wrote: > @ErrorType Done. https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:594: if (mShouldRequestExpirationDate) { On 2016/12/08 18:59:59, rouslan wrote: > Single line? Done. https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:624: */ On 2016/12/08 18:59:59, rouslan wrote: > @ErrorType > private int getExpirationDateErrorType() { Done. https://codereview.chromium.org/2557873002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:771: * Returns -1 if the input is empty or otherwise not a valid month. On 2016/12/08 18:59:59, rouslan wrote: > You can't say "otherwise not a valid month", because this method can return 99, > for example. Maybe you wanted to say "input is not a number". Done.
lgtm % comments https://codereview.chromium.org/2557873002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2557873002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:508: if (mMonthInput.isFocused() && mMonthInput.getText().length() == 2) { EXPIRATION_FIELDS_LENGTH instead of 2? https://codereview.chromium.org/2557873002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:510: } else if (mYearInput.isFocused() && mYearInput.getText().length() == 2) { Ditto
lgtm https://codereview.chromium.org/2557873002/diff/120001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2557873002/diff/120001/components/autofill_st... components/autofill_strings.grdp:237: Check your expiration date and try again This is actually only shown when the expiration date has passed. Can we be more specific in the message? Also, I don't like "try again". It's more like "please fix it". If so, also update the "desc" attribute so that the translator knows the context. ^^ All of this can be verified with UX and updated in a further change.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2557873002/#ps140001 (title: "Used constant")
Thanks! https://codereview.chromium.org/2557873002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2557873002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:508: if (mMonthInput.isFocused() && mMonthInput.getText().length() == 2) { On 2016/12/08 20:28:48, rouslan wrote: > EXPIRATION_FIELDS_LENGTH instead of 2? Yup, sorry that sneaked in (-_-) https://codereview.chromium.org/2557873002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:510: } else if (mYearInput.isFocused() && mYearInput.getText().length() == 2) { On 2016/12/08 20:28:48, rouslan wrote: > Ditto Done. https://codereview.chromium.org/2557873002/diff/120001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2557873002/diff/120001/components/autofill_st... components/autofill_strings.grdp:237: Check your expiration date and try again On 2016/12/08 20:55:17, Mathieu Perreault wrote: > This is actually only shown when the expiration date has passed. Can we be more > specific in the message? Also, I don't like "try again". It's more like "please > fix it". > > If so, also update the "desc" attribute so that the translator knows the > context. > > ^^ All of this can be verified with UX and updated in a further change. Thanks, will talk to you offline to make sure I understand and open a bug for that.
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sebsg@chromium.org
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": 140001, "attempt_start_ts": 1481301110910610,
"parent_rev": "5900b02b37aa9607b70a836f0611b5687db57d49", "commit_rev":
"88835793eac7a3450b1118e4bf0319bbb348dc52"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focus to the cvc input field when the expiration year field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 ========== to ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focus to the cvc input field when the expiration year field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 Review-Url: https://codereview.chromium.org/2557873002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focus to the cvc input field when the expiration year field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 Review-Url: https://codereview.chromium.org/2557873002 ========== to ========== [Payments] Move focus to next field in expired cvc unmask prompt. Moves the focus to the expiration year field when the expiration month field gets filled with a second character and move the focus to the cvc input field when the expiration year field gets filled with a second character. Also adds more detailed error messages for invalid expiration month and year. BUG=671762 Committed: https://crrev.com/2f32a659f1da7d65a343bff11bb8544969ecf276 Cr-Commit-Position: refs/heads/master@{#437571} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2f32a659f1da7d65a343bff11bb8544969ecf276 Cr-Commit-Position: refs/heads/master@{#437571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
