Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592733002:
[Payments] Update billing address dropdown style to match the design (Closed)
|
Created:
4 years ago by gogerald1 Modified:
3 years, 11 months ago CC:
chromium-reviews, rouslan+payments_chromium.org, gogerald+paymentswatch_chromium.org, sebsg+paymentswatch_chromium.org, dcheng, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate billing address dropdown style to match the design
BUG=661357
Committed: https://crrev.com/05265d26d330f8d8331ce624cce2a9c4a0cd616d
Cr-Commit-Position: refs/heads/master@{#441480}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 10
Patch Set 3 : address comments #Patch Set 4 : pad at the end of the selected item view to avoid overlapping dropdown icon #
Messages
Total messages: 38 (26 generated)
Description was changed from ========== Update dropdown style draft draft BUG= ========== to ========== Update billing address dropdown style BUG=661357 ==========
Description was changed from ========== Update billing address dropdown style BUG=661357 ========== to ========== Update billing address dropdown style to match the design BUG=661357 ==========
Patchset #1 (id:1) has been deleted
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...
gogerald@chromium.org changed reviewers: + rouslan@chromium.org, tedchoc@chromium.org
Hi rouslan@ and tedchoc@, PTAL.
gogerald@chromium.org changed reviewers: + ianwen@chromium.org
Hi ianwen@ could you take a look since tedchoc@ is ooo.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but please get a review from Ian as well.
https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/payment_request_editor_dropdown.xml (right): https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/payment_request_editor_dropdown.xml:27: android:dropDownWidth="match_parent" /> Q: how does this look on tablets? On Nexus 10 will be tiny texts with huge blank space? https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java (right): https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java:78: View view = LayoutInflater.from(mContext).inflate( Oops. Always inflating sounds like a bad idea as it makes the whole adapter pointless. I recommend you create 3 types of items. One is the top element, and one is the bottom element, and the last one is the middle elements. If you override getViewTypeCount() and getItemViewType() correctly, then getDropDownView() should only give you convertView with the matching type. There are many such examples in Chrome. See https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
Patchset #2 (id:40001) has been deleted
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...
https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/payment_request_editor_dropdown.xml (right): https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/payment_request_editor_dropdown.xml:27: android:dropDownWidth="match_parent" /> On 2016/12/20 21:52:23, Ian Wen wrote: > Q: how does this look on tablets? On Nexus 10 will be tiny texts with huge blank > space? I think it's fine since the spinner width is large as well. Attached two screenshots from Nexus 7 on the bug. https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java (right): https://codereview.chromium.org/2592733002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java:78: View view = LayoutInflater.from(mContext).inflate( On 2016/12/20 21:52:23, Ian Wen wrote: > Oops. Always inflating sounds like a bad idea as it makes the whole adapter > pointless. > > I recommend you create 3 types of items. One is the top element, and one is the > bottom element, and the last one is the middle elements. If you override > getViewTypeCount() and getItemViewType() correctly, then getDropDownView() > should only give you convertView with the matching type. > > There are many such examples in Chrome. See > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Yes, unfortunately getViewTypeCount must return 1 for spinner if targeting LOLLIPOP (https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...) But I managed to achieve a similar result.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry Gerald I have many comments. :O https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/payment_request_dropdown_item.xml (right): https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/payment_request_dropdown_item.xml:7: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" 1. The wrapping here might not be necessary. TextView itself could carry a padding. 2. Even if there is a need for such wrapping, FrameLayout should be favored as it is lighter than LinearLayout. https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java (right): https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java:42: private static final int TOP_VIEW_ID = 1; If multiple type does not work in spinner, I think you can simply manipulate paddings for the first and last item in getDropDownView(). You need to make sure that every tweak you do to those special elements, you should undo them for other normal items. A tiny example can be found here http://stackoverflow.com/a/26240895/4371425. This approach should simplify the code much more as no id or convertable need to be introduced. Also it guarantees performance, as we are not creating new views for every position. https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java:83: View view = isViewConvertable Nit: The variable, "view", can be removed and the code could be shortened like this: if (!isViewConvertable) { convertView = LayoutInflater.from(mContext).inflate()...; } https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java (right): https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:73: adapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item); I think there is a better way here: 1. replace R.layout.multiline_spinner_item with R.layout.payment_request_dropdown_item. 2. remove #73. 3. in getDropDownView(), call super.getDropDownView() to get a view, and don't bother to inflate the view yourself. https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:96: mDropdown.setPadding(0, 0, 0, 0); Could this be done in xml? If so, let's do it there.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
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...
Hi Ian, please help take another look, https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/payment_request_dropdown_item.xml (right): https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/payment_request_dropdown_item.xml:7: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2016/12/21 23:04:24, Ian Wen wrote: > 1. The wrapping here might not be necessary. TextView itself could carry a > padding. > 2. Even if there is a need for such wrapping, FrameLayout should be favored as > it is lighter than LinearLayout. The wrap is to display texts in multiple lines. Spinner dropdown is displayed in a DropDownListView which displays the item in a single line if it is an instance of TextView. FrameLayout do seems better here, https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java (right): https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java:42: private static final int TOP_VIEW_ID = 1; On 2016/12/21 23:04:24, Ian Wen wrote: > If multiple type does not work in spinner, I think you can simply manipulate > paddings for the first and last item in getDropDownView(). > > You need to make sure that every tweak you do to those special elements, you > should undo them for other normal items. A tiny example can be found here > http://stackoverflow.com/a/26240895/4371425. > > This approach should simplify the code much more as no id or convertable need to > be introduced. Also it guarantees performance, as we are not creating new views > for every position. Done. https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/BillingAddressAdapter.java:83: View view = isViewConvertable On 2016/12/21 23:04:24, Ian Wen wrote: > Nit: The variable, "view", can be removed and the code could be shortened like > this: > if (!isViewConvertable) { > convertView = LayoutInflater.from(mContext).inflate()...; > } Done. https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java (right): https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:73: adapter.setDropDownViewResource(android.R.layout.simple_spinner_dropdown_item); On 2016/12/21 23:04:25, Ian Wen wrote: > I think there is a better way here: > 1. replace R.layout.multiline_spinner_item with > R.layout.payment_request_dropdown_item. > 2. remove #73. > 3. in getDropDownView(), call super.getDropDownView() to get a view, and don't > bother to inflate the view yourself. Not sure I understand this comment correctly. Here, multiline_spinner_item is for getView when dropdown is not showing, simple_spinner_drop_down_item is for getDropdownView when showing dropdown. They have different style. And I intend to change the billing address dropdown but not others (even though the spinner dropdown box changed a little bit), so I moved this setting inside. In addition, we did not extend ArrayAdapter in this case. https://codereview.chromium.org/2592733002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorDropdownField.java:96: mDropdown.setPadding(0, 0, 0, 0); On 2016/12/21 23:04:25, Ian Wen wrote: > Could this be done in xml? If so, let's do it there. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Ian, ptal of the patch 4 with a minor update,
lgtm Patchset 4 looks great!
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2592733002/#ps160001 (title: "pad at the end of the selected item view to avoid overlapping dropdown icon")
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": 160001, "attempt_start_ts": 1483565184271470,
"parent_rev": "e89ced5ddfb93b4d36e9bb204389e08138a61408", "commit_rev":
"dea2d29bcca92956951f324957f501cda57332b2"}
Message was sent while issue was closed.
Description was changed from ========== Update billing address dropdown style to match the design BUG=661357 ========== to ========== Update billing address dropdown style to match the design BUG=661357 Review-Url: https://codereview.chromium.org/2592733002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Update billing address dropdown style to match the design BUG=661357 Review-Url: https://codereview.chromium.org/2592733002 ========== to ========== Update billing address dropdown style to match the design BUG=661357 Committed: https://crrev.com/05265d26d330f8d8331ce624cce2a9c4a0cd616d Cr-Commit-Position: refs/heads/master@{#441480} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/05265d26d330f8d8331ce624cce2a9c4a0cd616d Cr-Commit-Position: refs/heads/master@{#441480} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
