Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834073004:
Refine the layout of omnibox answer suggestions. (Closed)
|
Created:
3 years, 8 months ago by Justin Donnelly Modified:
3 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, jdonnelly+watch_chromium.org, tfarina, tommycli Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefine the layout of omnibox answer suggestions.
BUG=688408
Review-Url: https://codereview.chromium.org/2834073004
Cr-Commit-Position: refs/heads/master@{#467038}
Committed: https://chromium.googlesource.com/chromium/src/+/acba7d57f5705edc143bc9a3f864ced5e411e138
Patch Set 1 #
Total comments: 7
Patch Set 2 : Respond to comments. #
Messages
Total messages: 16 (9 generated)
The CQ bit was checked by jdonnelly@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...
jdonnelly@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:722: // Regardless of the text size, we ensure a minimum size for the content line This is my understanding of what this logic is trying to accomplish. Please let me know if I'm missing some subtlety.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
* I wonder if any tests make sense here (e.g. of the 0, 1, n-line answer cases getting the correct amount of spacing). * I wonder if the result view should be using a layout manager instead of a bunch of hand-rolled positioning. LGTM with those and other questions https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:51: // The padding that should be placed between lines in a top and bottom layout. What is "a top and bottom layout"? Do you mean "When a single result has more than one line of text"? Also, I wonder if instead of pulling this in multiple places below, it would make sense to use RenderText::SetMinLineHeight() on the answer rendertext. Of course, that will result in spacing out every line of text, not just placing space above "the whole answer"... maybe that's not what you want. https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:368: int answer_icon_size = GetAnswerLineHeight(); This will vertically center the icon against (however many answer lines you have) instead of against the center of the top line of the answer. Is that really what you want? https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:704: int OmniboxResultView::GetAnswerLineHeight() const { Nit: Seems like GetAnswerHeight() would be better both from "parallel structure" and "it can be more than one line" perspectives.
https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right): https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:51: // The padding that should be placed between lines in a top and bottom layout. On 2017/04/24 22:16:05, Peter Kasting wrote: > What is "a top and bottom layout"? Do you mean "When a single result has more > than one line of text"? "Top and bottom" is meant to distinguish a layout where the content and description are laid out vertically rather than side by side or horizontally. We're going to be experimenting with having other suggestions than answers use this layout so I wanted to introduce more general terminology. Now that I say it out loud "vertical layout" sounds much better. Switched to that and changed the wording to hopefully make this clearer. > Also, I wonder if instead of pulling this in multiple places below, it would > make sense to use RenderText::SetMinLineHeight() on the answer rendertext. Of > course, that will result in spacing out every line of text, not just placing > space above "the whole answer"... maybe that's not what you want. This is the spacing between the content and description rendertexts. It's confusing because these have been referred to as "lines" in this class but now the description can be multiline. Suggestions on making this clearer are welcome. https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:368: int answer_icon_size = GetAnswerLineHeight(); On 2017/04/24 22:16:05, Peter Kasting wrote: > This will vertically center the icon against (however many answer lines you > have) instead of against the center of the top line of the answer. Is that > really what you want? Yes, that's what I want. With multiple lines of text, the line height will be small. https://codereview.chromium.org/2834073004/diff/1/chrome/browser/ui/views/omn... chrome/browser/ui/views/omnibox/omnibox_result_view.cc:704: int OmniboxResultView::GetAnswerLineHeight() const { On 2017/04/24 22:16:05, Peter Kasting wrote: > Nit: Seems like GetAnswerHeight() would be better both from "parallel structure" > and "it can be more than one line" perspectives. Good point, done. Also changed CreateAnswerLine() to CreateAnswerText() to try to eliminate the old "line" terminology.
On 2017/04/24 22:16:05, Peter Kasting wrote: > * I wonder if any tests make sense here (e.g. of the 0, 1, n-line answer cases > getting the correct amount of spacing). > * I wonder if the result view should be using a layout manager instead of a > bunch of hand-rolled positioning. Yes, both of these sgtm. Tommy's going to be making bigger changes to this class soon as part of the suggestions UI experimentation. I think that'll be a good opportunity to switch to a layout manager and figure out how to effectively test this class (currently there are no tests).
The CQ bit was checked by jdonnelly@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2834073004/#ps20001 (title: "Respond to 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": 20001, "attempt_start_ts": 1493139401485510,
"parent_rev": "42f0a6a9bafa1e8ac02be1fdb8c5cf606e1021ee", "commit_rev":
"acba7d57f5705edc143bc9a3f864ced5e411e138"}
Message was sent while issue was closed.
Description was changed from ========== Refine the layout of omnibox answer suggestions. BUG=688408 ========== to ========== Refine the layout of omnibox answer suggestions. BUG=688408 Review-Url: https://codereview.chromium.org/2834073004 Cr-Commit-Position: refs/heads/master@{#467038} Committed: https://chromium.googlesource.com/chromium/src/+/acba7d57f5705edc143bc9a3f864... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/acba7d57f5705edc143bc9a3f864... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
