Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1924463002:
Align suggestion text with icon. (Closed)
|
Created:
4 years, 8 months ago by Kevin Bailey Modified:
4 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAlign suggestion text with icon.
Text was a tad high, since icon was moved down.
Parent bug: 591803.
BUG=610321
Committed: https://crrev.com/acc5363c0ee839d955b7bb26054f0d4611d0c3c7
Cr-Commit-Position: refs/heads/master@{#392922}
Patch Set 1 #Patch Set 2 : Created named constant #
Total comments: 4
Patch Set 3 : Making derivation of constants more obvious #
Total comments: 2
Patch Set 4 : Don't offset unless second line has image #Messages
Total messages: 28 (9 generated)
Description was changed from ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. BUG= ========== to ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. BUG= ==========
krb@chromium.org changed reviewers: + jdonnelly@chromium.org
Alternatively, we could only move the icon down by half as much (line 746), but I suspect it's where the author wanted it. btw, is there a bug?
On 2016/04/26 17:07:29, Kevin Bailey wrote: > Alternatively, we could only move the icon down by half as > much (line 746), but I suspect it's where the author > wanted it. As I recall, I was arbitrarily positioning the icon to align with the text. So moving the icon to account for the fact that the text is now smaller makes more sense to me. Let me know if you're seeing something in how it looks that motivated moving the text instead of the icon. > btw, is there a bug? I used https://crbug.com/591803 since this is related to the new text types.
Description was changed from ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. BUG= ========== to ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. BUG=591803 ==========
Added bug ID and named constant.
lgtm with a couple nits https://codereview.chromium.org/1924463002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java (right): https://codereview.chromium.org/1924463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java:65: private static final int ANSWER_IMAGE_VERTICAL_SPACING_DP = 5; Sorry, I know I'm being finicky about a really small change, but how about making this 2 and then adding both this and ANSWER_LINE2_VERTICAL_SPACING_DP to the mAnswerImage offset? That way it's clear that the image is part of the second line and is being shifted down with it, plus an extra factor? https://codereview.chromium.org/1924463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java:732: + ANSWER_LINE2_VERTICAL_SPACING_DP This spacing doesn't look right to me. Did you/could you run this through git cl format to check?
https://codereview.chromium.org/1924463002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java (right): https://codereview.chromium.org/1924463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java:65: private static final int ANSWER_IMAGE_VERTICAL_SPACING_DP = 5; On 2016/04/28 16:02:41, Justin Donnelly wrote: > Sorry, I know I'm being finicky about a really small change, but how about > making this 2 and then adding both this and ANSWER_LINE2_VERTICAL_SPACING_DP to > the mAnswerImage offset? That way it's clear that the image is part of the > second line and is being shifted down with it, plus an extra factor? This is definitely getting into "how I would have done it" territory. I'd rather do the addition here, so as not to clutter the code, but still retain the origin of the offset. Let me know if you'd really prefer the other way. https://codereview.chromium.org/1924463002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java:732: + ANSWER_LINE2_VERTICAL_SPACING_DP On 2016/04/28 16:02:41, Justin Donnelly wrote: > This spacing doesn't look right to me. Did you/could you run this through git cl > format to check? I've long since given up on trying to eyeball what "proper" style is. For this reason, I run: % git cl format; git cl upload
krb@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Ted, Can I get an owner approval? thx, krb
https://codereview.chromium.org/1924463002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java (right): https://codereview.chromium.org/1924463002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java:732: verticalOffset += line1Height should this only apply to answers?, should we be checking mSuggestion.hasAnswer() or something in the if?
https://codereview.chromium.org/1924463002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java (right): https://codereview.chromium.org/1924463002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java:732: verticalOffset += line1Height On 2016/04/28 18:31:58, Ted C wrote: > should this only apply to answers?, should we be checking > mSuggestion.hasAnswer() or something in the if? Ted, We looked at it with just a URL in the second line and it was fine, however it seems to me like it doesn't hurt to check. Justin, What do you think? I could check like so, or we could just not offset the image as much, like we discussed. I recall you like the spacing.
Ted, consider this a proposal. The sequence of events went like this: - The text in the second line of answers got smaller. - So we wanted to adjust the position of the image to align with the new text. - Kevin noticed that moving the image up made it crowd the first line of text. - When looking at second line positioning in general, we noticed that second line text looked cramped to us in general, e.g. in URL suggestions the second line is very close to the first line (the only spacing is the space in the labels themselves). So we thought it might make sense to add a small spacer for all 2-line suggestions. But if you don't like it, we can do it just for answers.
On 2016/04/28 20:33:42, Kevin Bailey wrote: > https://codereview.chromium.org/1924463002/diff/40001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java > (right): > > https://codereview.chromium.org/1924463002/diff/40001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/SuggestionView.java:732: > verticalOffset += line1Height > On 2016/04/28 18:31:58, Ted C wrote: > > should this only apply to answers?, should we be checking > > mSuggestion.hasAnswer() or something in the if? > > Ted, > > We looked at it with just a URL in the second line and it was fine, however it > seems to me like it doesn't hurt to check. > > Justin, > > What do you think? I could check like so, or we could just not offset the image > as much, like we discussed. I recall you like the spacing. If we want to apply it across the board, the variable descriptions shouldn't be so answer-y. Would be nice to include some screenshots in the bug if we want to do it everywhere.
On 2016/04/28 20:40:01, Justin Donnelly wrote: > Ted, consider this a proposal. The sequence of events went like this: > > - The text in the second line of answers got smaller. > - So we wanted to adjust the position of the image to align with the new text. > - Kevin noticed that moving the image up made it crowd the first line of text. > - When looking at second line positioning in general, we noticed that second > line text looked cramped to us in general, e.g. in URL suggestions the second > line is very close to the first line (the only spacing is the space in the > labels themselves). > > So we thought it might make sense to add a small spacer for all 2-line > suggestions. But if you don't like it, we can do it just for answers. The only thing that I'm worried about is that if we start spreading them apart more then it will be harder all the suggestions will get muddled together (there is already no separator, so visual compactness is really the best grouping we've got). Let's look at some comparison screenshots and that will be easier to answer IMO.
On 2016/04/28 20:42:57, Ted C wrote: > > The only thing that I'm worried about is that if we start spreading them > apart more then it will be harder all the suggestions will get muddled > together (there is already no separator, so visual compactness is really > the best grouping we've got). > > Let's look at some comparison screenshots and that will be easier to answer > IMO. I added the pix to the bug, with a little description for each. Let me know if they're not clear.
I was actually hoping to see a side by side for the links one, and I think we're missing the pre-your change link only image. On Thu, Apr 28, 2016 at 2:00 PM, <krb@chromium.org> wrote: > On 2016/04/28 20:42:57, Ted C wrote: > > > > The only thing that I'm worried about is that if we start spreading them > > apart more then it will be harder all the suggestions will get muddled > > together (there is already no separator, so visual compactness is really > > the best grouping we've got). > > > > Let's look at some comparison screenshots and that will be easier to > answer > > IMO. > > I added the pix to the bug, with a little description for each. Let me know > if they're not clear. > > https://codereview.chromium.org/1924463002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Done. On Thu, Apr 28, 2016 at 2:26 PM, Ted Choc <tedchoc@chromium.org> wrote: > I was actually hoping to see a side by side for the links one, and I think > we're missing the pre-your change link only image. > > On Thu, Apr 28, 2016 at 2:00 PM, <krb@chromium.org> wrote: > >> On 2016/04/28 20:42:57, Ted C wrote: >> > >> > The only thing that I'm worried about is that if we start spreading them >> > apart more then it will be harder all the suggestions will get muddled >> > together (there is already no separator, so visual compactness is really >> > the best grouping we've got). >> > >> > Let's look at some comparison screenshots and that will be easier to >> answer >> > IMO. >> >> I added the pix to the bug, with a little description for each. Let me >> know >> if they're not clear. >> >> https://codereview.chromium.org/1924463002/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. BUG=591803 ========== to ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. Parent bug: 591803. BUG=610321 ==========
On 2016/04/28 21:35:05, Kevin Bailey wrote: > Done. > > On Thu, Apr 28, 2016 at 2:26 PM, Ted Choc <mailto:tedchoc@chromium.org> wrote: > > > I was actually hoping to see a side by side for the links one, and I think > > we're missing the pre-your change link only image. > > > > On Thu, Apr 28, 2016 at 2:00 PM, <mailto:krb@chromium.org> wrote: > > > >> On 2016/04/28 20:42:57, Ted C wrote: > >> > > >> > The only thing that I'm worried about is that if we start spreading them > >> > apart more then it will be harder all the suggestions will get muddled > >> > together (there is already no separator, so visual compactness is really > >> > the best grouping we've got). > >> > > >> > Let's look at some comparison screenshots and that will be easier to > >> answer > >> > IMO. > >> > >> I added the pix to the bug, with a little description for each. Let me > >> know > >> if they're not clear. > >> > >> https://codereview.chromium.org/1924463002/ > >> > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I split off the discussion into crbug/610321. Can we re-start this and hopefully finish it up?
lgtm
The CQ bit was checked by krb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdonnelly@chromium.org Link to the patchset: https://codereview.chromium.org/1924463002/#ps60001 (title: "Don't offset unless second line has image")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924463002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924463002/60001
Message was sent while issue was closed.
Description was changed from ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. Parent bug: 591803. BUG=610321 ========== to ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. Parent bug: 591803. BUG=610321 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. Parent bug: 591803. BUG=610321 ========== to ========== Align suggestion text with icon. Text was a tad high, since icon was moved down. Parent bug: 591803. BUG=610321 Committed: https://crrev.com/acc5363c0ee839d955b7bb26054f0d4611d0c3c7 Cr-Commit-Position: refs/heads/master@{#392922} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/acc5363c0ee839d955b7bb26054f0d4611d0c3c7 Cr-Commit-Position: refs/heads/master@{#392922} |
