Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2429703003:
[PaymentRequest] Show errors and hide keyboard for opening card editor to fix required fields (Closed)
|
Created:
4 years, 2 months ago by gogerald1 Modified:
4 years, 2 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow errors and hide keyboard for opening card editor to fix required fields
BUG=656738
Committed: https://crrev.com/a99e608c125e3f5dc1437bf1bbf299c75762c4ad
Cr-Commit-Position: refs/heads/master@{#426081}
Patch Set 1 #
Total comments: 5
Patch Set 2 : address nit #Messages
Total messages: 24 (16 generated)
The CQ bit was checked by gogerald@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 gogerald@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:1) has been deleted
Description was changed from ========== draft draft BUG= ========== to ========== Show errors and hide keyboard for opening card editor to fix required information BUG=656738 ==========
The CQ bit was checked by gogerald@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...
Description was changed from ========== Show errors and hide keyboard for opening card editor to fix required information BUG=656738 ========== to ========== Show errors and hide keyboard for opening card editor to fix required fields BUG=656738 ==========
gogerald@chromium.org changed reviewers: + rouslan@chromium.org
Hi, PTAL,
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:445: WindowManager.LayoutParams.SOFT_INPUT_STATE_ALWAYS_VISIBLE); The keyboard is already showing by default. Is there a need to set it VISIBLE here? https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:479: if (field.isRequired() && field.isValid()) { Android Java style nit: if (field.isRequired() && field.isValid()) return true;
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:445: WindowManager.LayoutParams.SOFT_INPUT_STATE_ALWAYS_VISIBLE); On 2016/10/18 21:22:35, rouslan wrote: > The keyboard is already showing by default. Is there a need to set it VISIBLE > here? Yes, we need, this EditorView is hold by PaymentRequestUI, the Window will be reuse before PaymentRequestUI dismiss. https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:479: if (field.isRequired() && field.isValid()) { On 2016/10/18 21:22:35, rouslan wrote: > Android Java style nit: > > if (field.isRequired() && field.isValid()) return true; Acknowledged. Does Android use different java style as Google?, like 4.1.1 in https://engdoc.corp.google.com/eng/doc/devguide/java/styleguide.shtml?cl=head...
lgtm https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java (right): https://codereview.chromium.org/2429703003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorView.java:479: if (field.isRequired() && field.isValid()) { On 2016/10/18 21:37:39, gogerald1 wrote: > On 2016/10/18 21:22:35, rouslan wrote: > > Android Java style nit: > > > > if (field.isRequired() && field.isValid()) return true; > > Acknowledged. Does Android use different java style as Google?, like 4.1.1 in > https://engdoc.corp.google.com/eng/doc/devguide/java/styleguide.shtml?cl=head... Yes, Chrome's Android Java code follows Android style guide in general. Of course, owners can be nit-picky. So, for example, dfalcantara@ reviewed most of my Java code here. He prefers fewer lines of code, if possible. See line 492 in this file for example: if (!findAll) break; Android's guidance is documented in https://source.android.com/source/code-style.html#use-standard-brace-style for reference: "If the entire conditional (the condition and the body) fit on one line, you may (but are not obligated to) put it all on one line."
Looks other places in this component already use this style, fix it to be consistent.
The CQ bit was checked by gogerald@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.
Description was changed from ========== Show errors and hide keyboard for opening card editor to fix required fields BUG=656738 ========== to ========== Show errors and hide keyboard for opening card editor to fix required fields BUG=656738 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Show errors and hide keyboard for opening card editor to fix required fields BUG=656738 ========== to ========== Show errors and hide keyboard for opening card editor to fix required fields BUG=656738 Committed: https://crrev.com/a99e608c125e3f5dc1437bf1bbf299c75762c4ad Cr-Commit-Position: refs/heads/master@{#426081} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a99e608c125e3f5dc1437bf1bbf299c75762c4ad Cr-Commit-Position: refs/heads/master@{#426081} |
