Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2664083007:
Always adds margin at the end of the label to handle case where label (Closed)
DescriptionUse the layout specified in dropdown_item.xml when label and sublabel are on the same line.
BUG=687395
Review-Url: https://codereview.chromium.org/2664083007
Cr-Commit-Position: refs/heads/master@{#447916}
Committed: https://chromium.googlesource.com/chromium/src/+/c30e599ae66e6b96a515ab4537aeeb01ff948456
Patch Set 1 #Patch Set 2 : Sets label's layout params to sublabel only when label and sublabel are not on the same line. #Patch Set 3 : Sets label's layout params to sublabel only when label and sublabel are not on the same line. #
Total comments: 2
Patch Set 4 : Creates new layout params for sublabel to handle view reuse. #Messages
Total messages: 18 (8 generated)
Description was changed from ========== Always adds margin at the end of the label to handle case where label and sublabel are on the same line. BUG=687395 ========== to ========== Always adds margin at the end of the label to handle case where label and sublabel are on the same line. BUG=687395 ==========
csashi@google.com changed reviewers: + estark@chromium.org
On 2017/02/01 19:31:52, csashi wrote: > mailto:csashi@google.com changed reviewers: > + mailto:estark@chromium.org Hi Emily, Can you please take a look and let me know if the margins are correct. Is the truncation of "Login not secure" to "Login not sec..." expected? For your reference: When #http-form-warning is enabled: https://drive.google.com/file/d/0B1qGSlvsjCymT0dXbnMxeWVHNmc/view?usp=sharing When #http-form-warning is disabled: https://drive.google.com/file/d/0B1qGSlvsjCymbTYtWmo2N3JmNTA/view?usp=sharing Thanks, -sashi.
On 2017/02/01 19:37:15, csashi wrote: > On 2017/02/01 19:31:52, csashi wrote: > > mailto:csashi@google.com changed reviewers: > > + mailto:estark@chromium.org > > Hi Emily, > Can you please take a look and let me know if the margins are correct. Is the > truncation of "Login not secure" to "Login not sec..." expected? > > For your reference: > When #http-form-warning is enabled: > https://drive.google.com/file/d/0B1qGSlvsjCymT0dXbnMxeWVHNmc/view?usp=sharing > > When #http-form-warning is disabled: > https://drive.google.com/file/d/0B1qGSlvsjCymbTYtWmo2N3JmNTA/view?usp=sharing > > Thanks, > -sashi. Thanks for fixing this. In your two screenshots, is one of them supposed to have #http-form-warning disabled? Because the warning shows up in both of them. The truncation with ellipses is fine for now. We want to make it overflow instead of truncating, but it was truncating on smaller devices before your change so I don't think your change caused that. The layout still looks off to me though. Our specs at go/fns-ui-spec show 10dp between the end of the "Learn more" link and the right side of the dropdown. Possibly this is because the sublabel is getting a weight of 1, whereas before it was getting a weight of 0?
On 2017/02/02 06:53:46, estark wrote: > On 2017/02/01 19:37:15, csashi wrote: > > On 2017/02/01 19:31:52, csashi wrote: > > > mailto:csashi@google.com changed reviewers: > > > + mailto:estark@chromium.org > > > > Hi Emily, > > Can you please take a look and let me know if the margins are correct. Is the > > truncation of "Login not secure" to "Login not sec..." expected? > > > > For your reference: > > When #http-form-warning is enabled: > > https://drive.google.com/file/d/0B1qGSlvsjCymT0dXbnMxeWVHNmc/view?usp=sharing > > > > When #http-form-warning is disabled: > > https://drive.google.com/file/d/0B1qGSlvsjCymbTYtWmo2N3JmNTA/view?usp=sharing > > > > Thanks, > > -sashi. > > Thanks for fixing this. In your two screenshots, is one of them supposed to have > #http-form-warning disabled? Because the warning shows up in both of them. > > The truncation with ellipses is fine for now. We want to make it overflow > instead of truncating, but it was truncating on smaller devices before your > change so I don't think your change caused that. > > The layout still looks off to me though. Our specs at go/fns-ui-spec show 10dp > between the end of the "Learn more" link and the right side of the dropdown. > Possibly this is because the sublabel is getting a weight of 1, whereas before > it was getting a weight of 0? Hi Emily, Can you take a look @ https://drive.google.com/file/d/0B1qGSlvsjCymR0hSY0M0QlY2V2s/view?usp=sharing This looks different from the screenshot in https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c17 but the image I uploaded has 10dp between the end of the "Learn more" link and the right side of the dropdown - so it looks correct to me. You are correct - I see the "Login not secure" even when I don't enable #http-form-warning. That could be a different issue. -sashi.
On 2017/02/02 18:46:26, csashi wrote: > On 2017/02/02 06:53:46, estark wrote: > > On 2017/02/01 19:37:15, csashi wrote: > > > On 2017/02/01 19:31:52, csashi wrote: > > > > mailto:csashi@google.com changed reviewers: > > > > + mailto:estark@chromium.org > > > > > > Hi Emily, > > > Can you please take a look and let me know if the margins are correct. Is > the > > > truncation of "Login not secure" to "Login not sec..." expected? > > > > > > For your reference: > > > When #http-form-warning is enabled: > > > > https://drive.google.com/file/d/0B1qGSlvsjCymT0dXbnMxeWVHNmc/view?usp=sharing > > > > > > When #http-form-warning is disabled: > > > > https://drive.google.com/file/d/0B1qGSlvsjCymbTYtWmo2N3JmNTA/view?usp=sharing > > > > > > Thanks, > > > -sashi. > > > > Thanks for fixing this. In your two screenshots, is one of them supposed to > have > > #http-form-warning disabled? Because the warning shows up in both of them. > > > > The truncation with ellipses is fine for now. We want to make it overflow > > instead of truncating, but it was truncating on smaller devices before your > > change so I don't think your change caused that. > > > > The layout still looks off to me though. Our specs at go/fns-ui-spec show 10dp > > between the end of the "Learn more" link and the right side of the dropdown. > > Possibly this is because the sublabel is getting a weight of 1, whereas before > > it was getting a weight of 0? > > Hi Emily, > Can you take a look @ > https://drive.google.com/file/d/0B1qGSlvsjCymR0hSY0M0QlY2V2s/view?usp=sharing > This looks different from the screenshot in > https://bugs.chromium.org/p/chromium/issues/detail?id=678479#c17 but the image I > uploaded has 10dp between the end of the "Learn more" link and the right side of > the dropdown - so it looks correct to me. Thanks, lgtm. > > You are correct - I see the "Login not secure" even when I don't enable > #http-form-warning. That could be a different issue. Ah, I forgot, this field trial is enabled for local builds by default. So this is working as intended. > > -sashi.
csashi@google.com changed reviewers: + tedchoc@chromium.org
Hi Ted, Can you please review and approve? Thanks,
Description was changed from ========== Always adds margin at the end of the label to handle case where label and sublabel are on the same line. BUG=687395 ========== to ========== Use the layout specified in dropdown_item.xml when label and sublabel are on the same line. BUG=687395 ==========
https://codereview.chromium.org/2664083007/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right): https://codereview.chromium.org/2664083007/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/DropdownAdapter.java:196: sublabelView.setLayoutParams(layoutParams); by conditionally overwriting the layout params, if this view is reused where the sublabel is on the same line, I think you'll get the wrong layout params. I think in the else case, you need to regenerate the default params.
Hi Ted, Thanks for the catch. Please take a look. Screenshot with latest change: https://drive.google.com/file/d/0B1qGSlvsjCymZWZKUDYxRlFIcFE/view?usp=sharing Thanks, -sashi. https://codereview.chromium.org/2664083007/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right): https://codereview.chromium.org/2664083007/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/DropdownAdapter.java:196: sublabelView.setLayoutParams(layoutParams); On 2017/02/02 23:19:22, Ted C wrote: > by conditionally overwriting the layout params, if this view is reused where the > sublabel is on the same line, I think you'll get the wrong layout params. I > think in the else case, you need to regenerate the default params. Done.
lgtm
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org Link to the patchset: https://codereview.chromium.org/2664083007/#ps60001 (title: "Creates new layout params for sublabel to handle view reuse.")
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": 60001, "attempt_start_ts": 1486080289792800,
"parent_rev": "659270eddf6ac7da00edc5d6c45385824d111909", "commit_rev":
"c30e599ae66e6b96a515ab4537aeeb01ff948456"}
Message was sent while issue was closed.
Description was changed from ========== Use the layout specified in dropdown_item.xml when label and sublabel are on the same line. BUG=687395 ========== to ========== Use the layout specified in dropdown_item.xml when label and sublabel are on the same line. BUG=687395 Review-Url: https://codereview.chromium.org/2664083007 Cr-Commit-Position: refs/heads/master@{#447916} Committed: https://chromium.googlesource.com/chromium/src/+/c30e599ae66e6b96a515ab4537ae... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c30e599ae66e6b96a515ab4537ae... |
