New issue
Advanced search Search tips

Issue 697647 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

Move layout and styling of text from OmniboxPopupMaterialViewController to OmniboxPopupMaterialRow

Project Member Reported by jdonnelly@chromium.org, Mar 1 2017

Issue description

This is more of a proposal than a "should do". I'd welcome feedback from anyone on the iOS UI team.

The text in omnibox suggestions is positioned and styled by having OmniboxPopupMaterialRow expose text labels and relying on OmniboxPopupMaterialViewController to style them and handle layout. A more natural division of responsibility, I propose, would be to have OmniboxPopupMaterialRow responsible for the management of its own subviews. I believe this could be accomplished by having the controller set up and then pass the two attributed strings and optionally images for the left and right sides (answer images and append buttons, respectively).

Note, however, that this should avoid passing domain objects (e.g. AutocompleteMatch, SuggestionAnswer) into OmniboxPopupMaterialRow, given the guidance from the discussion below (from http://go/chromerev/550127013). 

On 2016/12/13 15:46:03, Justin Donnelly wrote:
> On 2016/12/12 23:43:12, lpromero wrote:
> > On 2016/12/12 21:36:41, Justin Donnelly wrote:
> > > Instead of having this here, I think it'd be better to just set the profile on
> > > the ShippingAddressItem above and let that class derive this label and lookup
> > > the name and phone number. That way we can remove all knowledge of how autofill
> > > profiles work from this class and encapsulate that in ShippingAddressItem.
> > 
> > We are going the other way. We are trying to keep the items minimal and with as
> > few dependencies as possible, while making the controllers in control.
> > 
> > Note that with the future refactoring, knowing what an autofill profile looks
> > like will be removed from the controller and moved to the coordinator.
> 
> Out of curiosity, what's the reasoning behind this? I don't really mind, but it
> seems a little unusual to *not* allow some UI element to have a model it's based
> on and instead just feed it raw data to display. Put another way, given a nice
> abstraction that encapsulates a series of related data and knowledge of how to
> format that data for display (AutofillProfile) why would you not want to use
> that to pass data between elements of the system?

Decoupling. In an ideal world, the view and the view controller
shouldn't know about what is an autofill profile and all its
dependencies. We would create a mediator object that knows about both
worlds and is the hermetic gateway.
Now, we will have to draw a line somewhere, and it's not yet clear where
we will set it. go/bling-clean-skeleton
 
Cc: rohitrao@chromium.org
One benefit of this would be to make the logic clearer when we select between the standard and answers detail labels in accessibilityValue in OmniboxPopupMaterialRow. In https://codereview.chromium.org/2721383002/ I'm adding that selection logic but it's based on inferring which one was chosen by the controller.
Labels: Hotlist-Refactoring
Cc: jdonnelly@chromium.org
Owner: stkhapugin@chromium.org
Status: Started (was: Assigned)
Status: Assigned (was: Started)

Sign in to add a comment