Improve AiS stock answer readability |
|||||||
Issue descriptionCan we make stock answers more readable by doing some one-off formatting with superscript/sizing?
,
Jul 19
,
Jul 19
,
Aug 21
,
Aug 21
orinj: I vaguely recall we talked about how there would have to be a little bit of code refactoring to accommodate more colors for parts of answers in the Refresh UI. But I think it didn't look like that big of a change? Let me know how much effort you think making these adjustments will take.
,
Aug 21
It's one of those situations where some one-offs and TODOs accumulated with color setting happening here and there. I'm planning to clean this up at least enough to achieve the new colorings while moving in the direction of simplicity. I don't think it's a big effort, but these text objects are subtly stateful and a little quirky to work with sometimes, so it's possible some time may be required to avoid regression. Doesn't look too bad, though, I can probably have a CL working by tomorrow.
,
Aug 21
Well... the plot thickens. I have text "types" coming in which can be used to apply styling to text ranges. So the stock answers are already colored and bolded as in the redline spec. The trouble is that now answers other than stocks will be styled as well, according to the text types... for example, now Shaq is 7' 1" in *bold* not normal weight. So either we have to: * Go to the source and change the text types on answers (may be deep and have side effects, e.g. on old layout answers) * Or use some other means to detect stocks and only apply the styling in this case My question is: How "one-off" is this styling, really? In my experience, the future is unknown so it's better to do things within a consistent logical framework instead of accumulating special exceptions. But if it really is one-off, I can detect ANSWER_TYPE_FINANCE and ignore text types for all other answers. Other suggestions for how to reconcile the inconsistency are also welcome.
,
Aug 23
See crrev.com/c/1186130 . This text-type interpretation approach will support both the old and new style answers, and will be much more maintainable going forward. Getting it to this point was involved, but now that it's done, restyling new answers is simple. Adding the color changes and bold for stocks became easy, and changing others will be similar.
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/69f31703a3b680c0b03d8004c2fb88400bda37a2 commit 69f31703a3b680c0b03d8004c2fb88400bda37a2 Author: Orin Jaworski <orinj@chromium.org> Date: Tue Aug 28 00:16:03 2018 [omnibox] Simplify new answer styling with text type interpretation This CL unifies disparate bits of logic for styling new answers by interpreting the text field types provided on the answer JSON from server. It began with styling stock answers, but the TextType values could not be applied consistently to new answers without considering additional context like answer type and line rank. So instead of complicating presentation logic further, it was eliminated. Now the presentation style is decided once at parse time and only the newly interpreted text types are used to decide rendering properties for text ranges. See TextTypeNewAnswer for more. Some TODOs were resolved and parsing logic simplified as well. Bug: 864799 Change-Id: If7ad3e4c83cb0eee2b4053646dacf7c139e461fe Reviewed-on: https://chromium-review.googlesource.com/1186130 Reviewed-by: Trent Apted <tapted@chromium.org> Reviewed-by: Tommy Li <tommycli@chromium.org> Commit-Queue: Orin Jaworski <orinj@chromium.org> Cr-Commit-Position: refs/heads/master@{#586515} [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/cocoa/omnibox/omnibox_popup_cell_unittest.mm [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/views/omnibox/omnibox_result_view.cc [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/views/omnibox/omnibox_text_view.cc [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/chrome/browser/ui/views/omnibox/omnibox_text_view.h [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/autocomplete_match_type_unittest.cc [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/search_suggestion_parser.cc [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/suggestion_answer.cc [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/suggestion_answer.h [modify] https://crrev.com/69f31703a3b680c0b03d8004c2fb88400bda37a2/components/omnibox/browser/suggestion_answer_unittest.cc
,
Aug 28
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by bklmn@chromium.org
, Jul 17106 KB
106 KB View Download