Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2914503002:
align shipping option name to left and shipping cost to right on bottom sheet (Closed)
|
Created:
3 years, 6 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. |
DescriptionSplit shipping option name and cost on bottom sheet.
On payment request bottom sheet, this change will align shipping
cost to the right, such that it will be on the same column as
total cost, and should be easier for user to scan through quickly.
BUG=716055
Review-Url: https://codereview.chromium.org/2914503002
Cr-Commit-Position: refs/heads/master@{#477711}
Committed: https://chromium.googlesource.com/chromium/src/+/bd06835c3db030fb9ffeff153df5d75006192d10
Patch Set 1 #Patch Set 2 : formatting #Patch Set 3 : formatting #
Total comments: 26
Patch Set 4 : address comments #
Total comments: 22
Patch Set 5 : more comments addressing #
Total comments: 10
Patch Set 6 : address comments #
Total comments: 2
Patch Set 7 : more fixing #
Messages
Total messages: 45 (33 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.
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: 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 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...
Description was changed from ========== align shipping option name to left and shipping cost to right on bottom sheet more polishing more polishing add tests for shipping options tweaking formatting sort out single/multi line stuff setting up integration test for shipping address/methods separation BUG=716055 ========== to ========== On payment request bottom sheet, this change will align shipping cost to the right, such that it will be on the same column as total cost, and should be easier for user to scan through quickly. BUG=716055 ==========
wuandy@chromium.org changed reviewers: + gogerald@google.com
Description was changed from ========== On payment request bottom sheet, this change will align shipping cost to the right, such that it will be on the same column as total cost, and should be easier for user to scan through quickly. BUG=716055 ========== to ========== On payment request bottom sheet, this change will align shipping cost to the right, such that it will be on the same column as total cost, and should be easier for user to scan through quickly. BUG=716055 ==========
please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wuandy@chromium.org changed reviewers: + gogerald@chromium.org - gogerald@google.com
good stuff, few comments, https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1066: private boolean mSeparateMainAndSubLabelInNormalMode = false; "=false" is not needed here, https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1066: private boolean mSeparateMainAndSubLabelInNormalMode = false; To make this feature more general instead of putting too much data specific control, it looks better to name it "mSplitSummaryInNormalMode" with comments "Indicates whether the summary should be split to display in the left and right summary text views in {@link DISPLAY_MODE_NORMAL}." or similar https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1181: * @param separateMainAndSubLabelInNormalMode set to true if the main label and the splitSummary? https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1183: * should be displayed seprately when in "If true, sets to split and display the summary in the left and right summary text views in {@link #DISPLAY_MODE_NORMAL}, otherwise the entire summary is displayed in the left summary text view. Note that the summary is split into 'label' and the rest ('sublabel, 'Tertiary label' and so on)." or similar https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1186: public void setSeparateMainAndSubLabelInNormalMode( setSplitSummaryInNormalMode()? https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1308: // Align main label to the left and sub label to the right if caller specified and no need to comment on alignment, the left summary text view is set to occupy the entire available space so as to push the right summary text view to the end. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1313: convertOptionToString(selectedItem, "setSummaryText(selectedItem.getLabel(), convertOptionToString(selectedItem, true /* excludeMainLabel */, false /* useBoldLabel */, mSummaryInSingleLine));" ? https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1373: private CharSequence convertOptionToString(PaymentOption item, boolean mainLabelOnly, excludeMainLabel? https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1425: public TextView getSummaryLabelForTest() { rename it to getLeftSummaryLabelForTest, https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1433: public TextView getSummarySubLabelForTest() { rename it to getRightSummaryLabelForTest https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:546: // Align shipping option name to the left and cost to the right. suggest "//Lets the selected shipping option name displays in the left summary text view and the cost displays in the right summary text view on bottom sheet." or similar? https://codereview.chromium.org/2914503002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2914503002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:78: "Free global shipping", mPaymentRequestTestRule.getShippingOptionSummaryLabel()); one space line below https://codereview.chromium.org/2914503002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:79: // Make sure shipping cost is aligned to the right on bottom sheet. You are not checking alignment here. Suggest: "// Make sure the shipping cost is displayed in the right summary text view on the bottom sheet."
Sorry, forgot to mention two more things, First, please add a short summary of the CL in the description as the first separate sentence which will be used to preview changes, like here https://chromium.googlesource.com/chromium/src/. Usually we just copy the CL title there. Second, please post a screenshot on the bug and ask bbergher@ to double confirm it.
Description was changed from ========== On payment request bottom sheet, this change will align shipping cost to the right, such that it will be on the same column as total cost, and should be easier for user to scan through quickly. BUG=716055 ========== to ========== Split shipping option name and cost on bottom sheet. On payment request bottom sheet, this change will align shipping cost to the right, such that it will be on the same column as total cost, and should be easier for user to scan through quickly. BUG=716055 ==========
https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1066: private boolean mSeparateMainAndSubLabelInNormalMode = false; On 2017/05/31 20:34:47, gogerald1 wrote: > To make this feature more general instead of putting too much data specific > control, it looks better to name it "mSplitSummaryInNormalMode" with comments > "Indicates whether the summary should be split to display in the left and right > summary text views in {@link DISPLAY_MODE_NORMAL}." or similar Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1066: private boolean mSeparateMainAndSubLabelInNormalMode = false; On 2017/05/31 20:34:47, gogerald1 wrote: > "=false" is not needed here, Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1181: * @param separateMainAndSubLabelInNormalMode set to true if the main label and the On 2017/05/31 20:34:47, gogerald1 wrote: > splitSummary? Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1183: * should be displayed seprately when in On 2017/05/31 20:34:47, gogerald1 wrote: > "If true, sets to split and display the summary in the left and right summary > text views in {@link #DISPLAY_MODE_NORMAL}, otherwise the entire summary is > displayed in the left summary text view. Note that the summary is split into > 'label' and the rest ('sublabel, 'Tertiary label' and so on)." or similar Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1186: public void setSeparateMainAndSubLabelInNormalMode( On 2017/05/31 20:34:47, gogerald1 wrote: > setSplitSummaryInNormalMode()? Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1308: // Align main label to the left and sub label to the right if caller specified and On 2017/05/31 20:34:47, gogerald1 wrote: > no need to comment on alignment, the left summary text view is set to occupy the > entire available space so as to push the right summary text view to the end. Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1313: convertOptionToString(selectedItem, On 2017/05/31 20:34:47, gogerald1 wrote: > "setSummaryText(selectedItem.getLabel(), convertOptionToString(selectedItem, > true /* excludeMainLabel */, false /* useBoldLabel */, mSummaryInSingleLine));" > ? changed to excludeMainLabel. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1373: private CharSequence convertOptionToString(PaymentOption item, boolean mainLabelOnly, On 2017/05/31 20:34:47, gogerald1 wrote: > excludeMainLabel? Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1425: public TextView getSummaryLabelForTest() { On 2017/05/31 20:34:47, gogerald1 wrote: > rename it to getLeftSummaryLabelForTest, Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1433: public TextView getSummarySubLabelForTest() { On 2017/05/31 20:34:47, gogerald1 wrote: > rename it to getRightSummaryLabelForTest Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/2914503002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:546: // Align shipping option name to the left and cost to the right. On 2017/05/31 20:34:47, gogerald1 wrote: > suggest "//Lets the selected shipping option name displays in the left summary > text view and the cost displays in the right summary text view on bottom sheet." > or similar? Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2914503002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:78: "Free global shipping", mPaymentRequestTestRule.getShippingOptionSummaryLabel()); On 2017/05/31 20:34:47, gogerald1 wrote: > one space line below Done. https://codereview.chromium.org/2914503002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:79: // Make sure shipping cost is aligned to the right on bottom sheet. On 2017/05/31 20:34:47, gogerald1 wrote: > You are not checking alignment here. > > Suggest: "// Make sure the shipping cost is displayed in the right summary text > view on the bottom sheet." 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (left): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1356: if (useBoldLabel) { useBoldLabel is used for main label only https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1065: * text views in {@link PaymentRequestSection.DISPLAY_MODE_NORMAL}. */ This is not a public interface or variable, so you can replace "{@link PaymentRequestSection.DISPLAY_MODE_NORMAL}" with "DISPLAY_MODE_NORMAL". In addition, if the comments have multiple lines, put '/**' and '*/' in a separate line without any text in that line, like below. /** * Indicates whether the summary should be split to display in the left and right summary * text views in DISPLAY_MODE_NORMAL. */ https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1066: private boolean mSplitSummaryInNormalMode; looks 'mSplitSummaryInDisplayModeNormal' is better, 'DisplayModeNormal' corresponds to 'DISPLAY_MODE_NORMAL' https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1181: * @param splitSummary set to true to split the display of summary in left and right Uppercase the first character to comment a param. In addition, the your comments is for the param but not the function, so "set to true to...." looks a little confusing. You can follow below pattern. /** * Function comments..... * * @param splitSummary If true, ....... Otherwise... */ https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1186: public void setSplitSummaryInNormalMode(boolean splitSummary) { setSplitSummaryInDisplayNormalMode? https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1308: // section is in DISPLAY_MODE_NORMAL // Split summary in DISPLAY_MODE_NORMAL if caller specified. The first part is displayed on the left summary text view aligned to the left. The second part is displayed on the right summary text view aligned to the right. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1430: * Returns the label of the summary right text view for the OptionSection. *the right summary text view* to be consistent https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:80: // Make sure shipping cost is display in the summary right text view. *is displayed in the right summary text view. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestCommon.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestCommon.java:426: protected String getShippingOptionCostSummary() throws ExecutionException { getShippingOptionCostSummaryOnBottomSheet? https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestRule.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestRule.java:280: protected String getShippingOptionCostSummaryLabel() throws ExecutionException { getShippingOptionCostSummaryLabelOnBottomSheet? and updates comments accordingly
https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (left): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1356: if (useBoldLabel) { On 2017/06/02 15:05:10, gogerald1 wrote: > useBoldLabel is used for main label only I think this is something better left to the callers to this function to decide. This function as a utility should only provide the options available, while the caller can set useBoldLabel=false while excludeMainLabel=true accordingly. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1065: * text views in {@link PaymentRequestSection.DISPLAY_MODE_NORMAL}. */ On 2017/06/02 15:05:10, gogerald1 wrote: > This is not a public interface or variable, so you can replace "{@link > PaymentRequestSection.DISPLAY_MODE_NORMAL}" with "DISPLAY_MODE_NORMAL". > > In addition, if the comments have multiple lines, put '/**' and '*/' in a > separate line without any text in that line, like below. > > /** > * Indicates whether the summary should be split to display in the left and > right summary > * text views in DISPLAY_MODE_NORMAL. > */ Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1066: private boolean mSplitSummaryInNormalMode; On 2017/06/02 15:05:10, gogerald1 wrote: > looks 'mSplitSummaryInDisplayModeNormal' is better, 'DisplayModeNormal' > corresponds to 'DISPLAY_MODE_NORMAL' Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1181: * @param splitSummary set to true to split the display of summary in left and right On 2017/06/02 15:05:10, gogerald1 wrote: > Uppercase the first character to comment a param. In addition, the your comments > is for the param but not the function, so "set to true to...." looks a little > confusing. You can follow below pattern. > > /** > * Function comments..... > * > * @param splitSummary If true, ....... Otherwise... > */ Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1186: public void setSplitSummaryInNormalMode(boolean splitSummary) { On 2017/06/02 15:05:10, gogerald1 wrote: > setSplitSummaryInDisplayNormalMode? Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1308: // section is in DISPLAY_MODE_NORMAL On 2017/06/02 15:05:10, gogerald1 wrote: > // Split summary in DISPLAY_MODE_NORMAL if caller specified. The first part is > displayed on the left summary text view aligned to the left. The second part is > displayed on the right summary text view aligned to the right. Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1430: * Returns the label of the summary right text view for the OptionSection. On 2017/06/02 15:05:10, gogerald1 wrote: > *the right summary text view* to be consistent Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:80: // Make sure shipping cost is display in the summary right text view. On 2017/06/02 15:05:11, gogerald1 wrote: > *is displayed in the right summary text view. Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestCommon.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestCommon.java:426: protected String getShippingOptionCostSummary() throws ExecutionException { On 2017/06/02 15:05:11, gogerald1 wrote: > getShippingOptionCostSummaryOnBottomSheet? Done. https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestRule.java (right): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestRule.java:280: protected String getShippingOptionCostSummaryLabel() throws ExecutionException { On 2017/06/02 15:05:11, gogerald1 wrote: > getShippingOptionCostSummaryLabelOnBottomSheet? and updates comments accordingly 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (left): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1356: if (useBoldLabel) { On 2017/06/02 19:53:12, wuandy1 wrote: > On 2017/06/02 15:05:10, gogerald1 wrote: > > useBoldLabel is used for main label only > > I think this is something better left to the callers to this function to decide. > This > function as a utility should only provide the options available, while the > caller can > set useBoldLabel=false while excludeMainLabel=true accordingly. Yes, but previously useBoldLabel is only apply for item.getLabel, we should keep this fact and let caller do it or not. However after your change, the span will be applied on an empty string sometimes, that doesn't make sense, so you might want to move this logic inside the if(!excludeMainLabel) check. https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1183: * Specify if the summary should be displayed split under DISPLAY_MODE_NORMAL. one space line below https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1183: * Specify if the summary should be displayed split under DISPLAY_MODE_NORMAL. Specify whether.........should be split to display under.... https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1184: * @param splitSummary set to true to split the display of summary in left and right If true, split the display..... or at least uppercase the first character "Set" https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1186: * summary is displayed in left text view. Note that summary is Move this sentence before 'Otherwise' and replace 'Note that' with 'The'. https://codereview.chromium.org/2914503002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2914503002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:80: // Make sure shipping cost is display in the right summary text view. is displayed...
The CQ bit was checked by wuandy@chromium.org to run a CQ dry run
https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (left): https://codereview.chromium.org/2914503002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1356: if (useBoldLabel) { On 2017/06/06 16:55:19, gogerald1 wrote: > On 2017/06/02 19:53:12, wuandy1 wrote: > > On 2017/06/02 15:05:10, gogerald1 wrote: > > > useBoldLabel is used for main label only > > > > I think this is something better left to the callers to this function to > decide. > > This > > function as a utility should only provide the options available, while the > > caller can > > set useBoldLabel=false while excludeMainLabel=true accordingly. > > Yes, but previously useBoldLabel is only apply for item.getLabel, we should keep > this fact and let caller do it or not. However after your change, the span will > be applied on an empty string sometimes, that doesn't make sense, so you might > want to move this logic inside the if(!excludeMainLabel) check. absolutely! good catch! https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1183: * Specify if the summary should be displayed split under DISPLAY_MODE_NORMAL. On 2017/06/06 16:55:19, gogerald1 wrote: > Specify whether.........should be split to display under.... Done. https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1183: * Specify if the summary should be displayed split under DISPLAY_MODE_NORMAL. On 2017/06/06 16:55:19, gogerald1 wrote: > Specify whether.........should be split to display under.... Done. https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1184: * @param splitSummary set to true to split the display of summary in left and right On 2017/06/06 16:55:19, gogerald1 wrote: > If true, split the display..... or at least uppercase the first character "Set" Done. https://codereview.chromium.org/2914503002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1186: * summary is displayed in left text view. Note that summary is On 2017/06/06 16:55:20, gogerald1 wrote: > Move this sentence before 'Otherwise' and replace 'Note that' with 'The'. Done. https://codereview.chromium.org/2914503002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java (right): https://codereview.chromium.org/2914503002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java:80: // Make sure shipping cost is display in the right summary text view. On 2017/06/06 16:55:20, gogerald1 wrote: > is displayed... Done.
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 % comments https://codereview.chromium.org/2914503002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1185: * @param splitSummary If true split the display of summary in left and right If true, ....in the left and right text views........is displayed in the left text view
https://codereview.chromium.org/2914503002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2914503002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:1185: * @param splitSummary If true split the display of summary in left and right On 2017/06/07 15:40:42, gogerald1 wrote: > If true, ....in the left and right text views........is displayed in the left > text view 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/2914503002/#ps120001 (title: "more fixing")
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": 120001, "attempt_start_ts": 1496853976384120,
"parent_rev": "6b41b1ee575bdf3bf738be8ac5db57f2e41476ff", "commit_rev":
"bd06835c3db030fb9ffeff153df5d75006192d10"}
Message was sent while issue was closed.
Description was changed from ========== Split shipping option name and cost on bottom sheet. On payment request bottom sheet, this change will align shipping cost to the right, such that it will be on the same column as total cost, and should be easier for user to scan through quickly. BUG=716055 ========== to ========== Split shipping option name and cost on bottom sheet. On payment request bottom sheet, this change will align shipping cost to the right, such that it will be on the same column as total cost, and should be easier for user to scan through quickly. BUG=716055 Review-Url: https://codereview.chromium.org/2914503002 Cr-Commit-Position: refs/heads/master@{#477711} Committed: https://chromium.googlesource.com/chromium/src/+/bd06835c3db030fb9ffeff153df5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/bd06835c3db030fb9ffeff153df5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
