Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2908483003:
separate shipping address and option on bottom sheet (Closed)
|
Created:
3 years, 7 months ago by wuandy1 Modified:
3 years, 6 months ago Reviewers:
gogerald1 CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, agrieve+watch_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionseparate shipping address and option on PR bottom sheet.
Detailed summary:
1. replace ShippingSummarySection with ShippingAddressSection and
ShippingOptionSection in bottom sheet.
2. ShippingAddressSection will display address in multiple lines.
3. Behaviors of expanded sheet is not changed.
Note: There will be another CL to align shipping code to the right.
This CL is already big.
BUG=716055
Review-Url: https://codereview.chromium.org/2908483003
Cr-Commit-Position: refs/heads/master@{#475373}
Committed: https://chromium.googlesource.com/chromium/src/+/f05d0f177c30e5ad1b0309b46b69a288336012a8
Patch Set 1 #Patch Set 2 : add tests for shipping options #Patch Set 3 : more polishing #
Total comments: 22
Patch Set 4 : address comments #
Total comments: 8
Patch Set 5 : fixing typo and syntax in comments #Patch Set 6 : more formatting #
Messages
Total messages: 28 (21 generated)
The CQ bit was checked by wuandy@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.
Description was changed from
==========
separate shipping address and option on bottom sheet
BUG=
==========
to
==========
separate shipping address and option on PR bottom sheet.
Detailed summary:
1. replace ShippingSummarySection with ShippingAddressSection and
ShippingOptionSection in bottom sheet.
2. ShippingAddressSection will display address in multiple lines.
3. Behaviors of expanded sheet is not changed.
Note: There will be another CL to align shipping code to the right.
This CL is already big.
BUG=716055
==========
wuandy@chromium.org changed reviewers: + gogerald@chromium.org
The CQ bit was checked by wuandy@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...
please review.
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 wuandy@chromium.org to run a CQ dry run
The CQ bit was checked by wuandy@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.
https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1057: private boolean mDisplaySingleLineSummaryInNormalMode; mSummaryInSingleLine is also used to remember summary display status so as to avoid setting it multiple times, so we may need a new boolean here. please update the comments to "Indicates whether the summary is displaying in a single line." for mSummaryInSingleLine, The new boolean could be "boolean mSetDisplaySummaryInSingleLineInNormalMode = true" with comments "Indicates whether the summary is set to display in a single line in DISPLAY_MODE_NORMAL by caller." or similar, https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1079: mDisplaySingleLineSummaryInNormalMode = true; you can set default value when defining it https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1164: * @param displaySingleLineSummaryInNormalMode set to true if the summary text should suggest: /** * @param singleLine If true, sets the summary text to display in a single line in {@link #DISPLAY_MODE_NORMAL} when there is a valid selected option, otherwise sets the summary text to display in multiple lines. */ https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1168: public void setDisplaySingleLineSummaryInNormalMode( setDisplaySummaryInSingleLineInNormalMode? https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1260: if (isDisplaySingleLine) { With the new boolean, you can keep the original code, but update "if (selectedItem == null || (mDisplayMode == DISPLAY_MODE_NORMAL && mSetDisplaySummaryInSingleLineInNormalMode))" and comments to "*or the display mode is DISPLAY_MODE_NORMAL without be set to display in multiple lines explicitly by caller." https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1390: * Returns the lable of the summary for the OptionSection. *label* You can simply say "Returns the label of the section summary." https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:543: // Display address in multiple lines as section summary on bottom sheet. "// Display summary of the selected address in multiple lines on bottom sheet."? https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:73: mPaymentRequestTestRule.getShippingAddressSummaryLabel()); one space line below https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:74: // Make sure shipping option summary on is as expected. "*on the bottomsheet is displayed in a single line as expected."? https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:95: .toString()); one space line below https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:96: // Make sure shipping option summary on is as expected(multi-line). "*on the fullsheet is displayed in multiple lines as expected."?
https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1057: private boolean mDisplaySingleLineSummaryInNormalMode; On 2017/05/29 15:55:59, gogerald1 wrote: > mSummaryInSingleLine is also used to remember summary display status so as to > avoid setting it multiple times, so we may need a new boolean here. > > please update the comments to "Indicates whether the summary is displaying in a > single line." for mSummaryInSingleLine, > > The new boolean could be "boolean mSetDisplaySummaryInSingleLineInNormalMode = > true" with comments "Indicates whether the summary is set to display in a single > line in DISPLAY_MODE_NORMAL by caller." or similar, > Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1079: mDisplaySingleLineSummaryInNormalMode = true; On 2017/05/29 15:56:00, gogerald1 wrote: > you can set default value when defining it Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1164: * @param displaySingleLineSummaryInNormalMode set to true if the summary text should On 2017/05/29 15:55:59, gogerald1 wrote: > suggest: > > /** > * @param singleLine If true, sets the summary text to display in a single line > in {@link #DISPLAY_MODE_NORMAL} when there is a valid selected option, otherwise > sets the summary text to display in multiple lines. > */ Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1168: public void setDisplaySingleLineSummaryInNormalMode( On 2017/05/29 15:55:59, gogerald1 wrote: > setDisplaySummaryInSingleLineInNormalMode? Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1260: if (isDisplaySingleLine) { On 2017/05/29 15:55:59, gogerald1 wrote: > With the new boolean, you can keep the original code, but update "if > (selectedItem == null || (mDisplayMode == DISPLAY_MODE_NORMAL && > mSetDisplaySummaryInSingleLineInNormalMode))" > > and comments to > > "*or the display mode is DISPLAY_MODE_NORMAL without be set to display in > multiple lines explicitly by caller." Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1390: * Returns the lable of the summary for the OptionSection. On 2017/05/29 15:55:59, gogerald1 wrote: > *label* > > You can simply say "Returns the label of the section summary." Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2908483003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:543: // Display address in multiple lines as section summary on bottom sheet. On 2017/05/29 15:56:00, gogerald1 wrote: > "// Display summary of the selected address in multiple lines on bottom sheet."? Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:73: mPaymentRequestTestRule.getShippingAddressSummaryLabel()); On 2017/05/29 15:56:00, gogerald1 wrote: > one space line below Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:74: // Make sure shipping option summary on is as expected. On 2017/05/29 15:56:00, gogerald1 wrote: > "*on the bottomsheet is displayed in a single line as expected."? Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:95: .toString()); On 2017/05/29 15:56:00, gogerald1 wrote: > one space line below Done. https://codereview.chromium.org/2908483003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:96: // Make sure shipping option summary on is as expected(multi-line). On 2017/05/29 15:56:00, gogerald1 wrote: > "*on the fullsheet is displayed in multiple lines as expected."? Done.
The CQ bit was checked by wuandy@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...
lgtm % comments https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1060: * by caller.*/ put '*/' in a separate line https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1255: // Summary is displayed in multiple line by default unless: *multiple lines* https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1257: // 2. display mode is DISPLAY_MODE_NORMAL without caller explicitly set to display *the display. https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:543: // Display summary of selected address in multiple lines on bottom sheet. Display the summary of the selected*
https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1060: * by caller.*/ On 2017/05/29 18:09:51, gogerald1 wrote: > put '*/' in a separate line Done. https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1255: // Summary is displayed in multiple line by default unless: On 2017/05/29 18:09:51, gogerald1 wrote: > *multiple lines* Done. https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1257: // 2. display mode is DISPLAY_MODE_NORMAL without caller explicitly set to display On 2017/05/29 18:09:51, gogerald1 wrote: > *the display. Done. https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2908483003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:543: // Display summary of selected address in multiple lines on bottom sheet. On 2017/05/29 18:09:51, gogerald1 wrote: > Display the summary of the selected* Done.
The CQ bit was checked by wuandy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gogerald@chromium.org Link to the patchset: https://codereview.chromium.org/2908483003/#ps80001 (title: "fixing typo and syntax in comments")
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": 80001, "attempt_start_ts": 1496081989475400,
"parent_rev": "d512ea39107a9cbad6936a9e98ea9214116433d8", "commit_rev":
"f05d0f177c30e5ad1b0309b46b69a288336012a8"}
Message was sent while issue was closed.
Description was changed from
==========
separate shipping address and option on PR bottom sheet.
Detailed summary:
1. replace ShippingSummarySection with ShippingAddressSection and
ShippingOptionSection in bottom sheet.
2. ShippingAddressSection will display address in multiple lines.
3. Behaviors of expanded sheet is not changed.
Note: There will be another CL to align shipping code to the right.
This CL is already big.
BUG=716055
==========
to
==========
separate shipping address and option on PR bottom sheet.
Detailed summary:
1. replace ShippingSummarySection with ShippingAddressSection and
ShippingOptionSection in bottom sheet.
2. ShippingAddressSection will display address in multiple lines.
3. Behaviors of expanded sheet is not changed.
Note: There will be another CL to align shipping code to the right.
This CL is already big.
BUG=716055
Review-Url: https://codereview.chromium.org/2908483003
Cr-Commit-Position: refs/heads/master@{#475373}
Committed:
https://chromium.googlesource.com/chromium/src/+/f05d0f177c30e5ad1b0309b46b69...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f05d0f177c30e5ad1b0309b46b69... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
