angular / components

Component infrastructure and Material Design components for Angular
https://material.angular.io
MIT License
24.34k stars 6.74k forks source link

*reduce* need for displayWith function on mat-autocomplete #8436

Open sarora2073 opened 6 years ago

sarora2073 commented 6 years ago

Bug, feature request, or proposal:

Proposal

What is the expected behavior?

A simple way to display the selected autocomplete value in an input field.

What is the current behavior?

Currently when you setup an autocomplete and bind a [value] to the , the autocomplete displays the value UNLESS you assign a displayWith function. However, it makes no sense to me to default to showing the [value] since that is intended to be the behind-the-scenes value, not the displayValue. If we simply default to displaying the viewValue somehow, we would no longer need a displayWith function, and that would eliminate numerous other headaches reported in other issues on this forum.

What are the steps to reproduce?

Providing a StackBlitz/Plunker (or similar) is the best way to get the team to see your issue.
Plunker starter (using on @master): https://goo.gl/uDmqyY
StackBlitz starter (using latest npm release): https://goo.gl/wwnhMV

This proposal relies on an understanding of the typical autocomplete implementation e.g. here: https://plnkr.co/edit/W9vdWaGcoG90LKM8hHgI?p=preview

What is the use-case or motivation for changing an existing behavior?

displayWith function has numerous issues reported. For my purpose I see it as a potential blocker to generating dynamic forms in particular, for reasons i'm glad to detail if need be but I'll spare at this time.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Is there anything else we should know?

i'm guessing the displayWith function was created in the first place to address a legitimate challenge. Namely, the autocomplete control needs a way to know what the displayValue is.

so in the demo example here:

<mat-form-field>
  <mat-select placeholder="State">
    <mat-option>None</mat-option>
    <mat-option *ngFor="let state of states" [value]="state.code">{{ state.name }}</mat-option>
  </mat-select>
</mat-form-field>

the displayValue is state.name. Perhaps the new syntax that requires no displayValue would be like this:

<mat-form-field>
  <mat-select placeholder="State">
    <mat-option>None</mat-option>
    <mat-option *ngFor="let state of states" [value]="state.code" [dislayValue]="state.name"></mat-option>
  </mat-select>
</mat-form-field>

Such a design would continue to allow flexibility to assign the whole object or a particular field to the [value]..which is important to various use cases out there.

I don't see any downside to eliminating the displayWith function as long as the viewValue is always shown I doubt people would care.

willshowell commented 6 years ago
<mat-option *ngFor="let state of states" [value]="state.code" [dislayValue]="state.name"></mat-option>

I disagree with limiting mat-option template to a single string representation (displayValue). It is very powerful and flexible to allow custom templating within mat-option, and I'd opine that feature is more important than the issues presented in https://github.com/angular/material2/issues/4863.

screen shot 2017-11-14 at 12 11 40 pm

I know I suggested just showing viewValue by default, but that can be troublesome when using icons of images in your mat-option, since viewValue is really just the option's innerText.

I could get behind a mat-autocomplete-trigger directive like mat-select has. It would give you access to the innerText of the selected option, but also support all the current behavior by letting you continue to use your displayWith method.

I could also get behind allowing slightly more context than just value to be added to mat-option. So instead of just binding a value, you can bind something like

<mat-option [value]="myOption.value" [representation]="myOption">

And when doing so, displayWith would be called with representation || value.


I see it as a potential blocker to generating dynamic forms in particular, for reasons i'm glad to detail if need be but I'll spare at this time.

Since dynamic can be used in a lot of different contexts, I'd be interested in what issues you're encountering.

sarora2073 commented 6 years ago

Thanks for the quick response.

I wasn't thinking of those advanced non-string displayValues...and that raises more questions e.g. does displayWith function support those anyway? Or is displayWith only compatible with string displayValues? if yes to the latter then maybe we're okay to support a binding like [viewValue] since when it's used we're only working with strings anyway? Just a thought.

re: the dynamic forms angle...i've been playing with ng-dynamic-forms library and it currently can't support displayWith dynamic creation. Upon reviewing the code it relies on a 'dynamic control component' to generate each control, and a parent form component that dynamically generates multiple controls.

Constraints:

..and this is how I ended up wishing displayWith wasn't a necessity, and wishing the displayValue would always show by default. If there's a way to show the selected option (not it's [value] without an explicit displayValue I guess that would be ideal given what you've said.

p.s. here's the related discussion on the ng-dynamic-forms library side if anyone is interested: https://github.com/udos86/ng-dynamic-forms/issues/355

sarora2073 commented 6 years ago

btw, just wanted to hone in on this part of your response:

I could get behind a mat-autocomplete-trigger directive like mat-select has. It would give you access to the innerText of the selected option, but also support all the current behavior by letting you continue to use your displayWith method.

Displaying innerText is what I also was trying to describe earlier...if innerText displayed by default I can't conceive of ever needing displayWith. Your answer suggests you would still want displayWith capability even after a re-design..curious as to when you think it might still be useful. Thanks.

willshowell commented 6 years ago

i've been playing with ng-dynamic-forms library and it currently can't support displayWith dynamic creation

Ehh IMO just because another wrapper library doesn't yet support displayWith does not justify its removal.

...if innerText displayed by default I can't conceive of ever needing displayWith... curious as to when you think it might still be useful.

Suppose you're using the example on the docs site,

<mat-option *ngFor="let state of states" [value]="state.name">
  <img ... />
  <span>{{ state.name }}</span> |
  <small>Population: {{state.population}}</small>
</mat-option>

The inner text of the Arkansas option would be " Arkansas | Population: 2.978M". I think it's reasonable that a user would want the trigger to simply read "Arkansas" instead of the full innerText. If they were able to template the trigger to their liking, they could just do

<mat-autocomplete-trigger>{{myDisplayWithFn(select.selected?.value)}}</mat-autocomplete-trigger>

or, as you said, just use the viewValue

<mat-autocomplete-trigger>{{ select.selected?.viewValue }}</mat-autocomplete-trigger>

I identify a few ways this could go to provide a better developer experience,

  1. Remove the ability to template inside mat-option (maybe that's totally valid! but it's also a breaking change and feature reduction)
  2. Allow users to template their own trigger like mat-select
  3. Provide more context to displayWith so that users don't have to cache huge arrays and filter through them each time they want to retrieve the original object.
  4. Maybe add a dataAccessor to mat-option so [value] can be bound to the full object, but the dataAccessor function can be used to set the NgControl? I have no idea if that could work, but it would also solve OP's problems in #4863.
sarora2073 commented 6 years ago

re: this:

Ehh IMO just because another wrapper library doesn't yet support displayWith does not justify its removal.

The problem isn't with this one library as it stands..i've created a fork of it here and have enhanced it to bind to objects, but supporting displayWith dynamically is the missing piece for it to work for my app. Currently my autocomplete fields show a mongodb ObjectId once a value is selected because of this limitation..ugh!

re: continuing to support displayWith even if the default display is the innerText, I get it now..that makes sense that the innerText isn't always the desired display value in the example you gave.

That said, I'd dont have anything to add about how to improve displayWith since I'm hoping not to rely on it for my use case. For those that would rely on it AND want dynamic forms, ideally the new displayWith approach would be dynamic forms friendly i.e. formbuilder should be able to programmatically set the displayWith function, rather than being restricted to define it in markup only.

kdubious commented 6 years ago

I'm getting tripped up on implementing what I think is a fairly standard use case for a combobox (autocomplete).

The list data comes from a backend service in a Key-Value pair format. {id:1, name:"One"}) I want to bind id, and show name. I also want to filter when I type.

Is this supported today?

SafeEval commented 6 years ago

An obvious solution to the id/name use case, described by @kdubious, would be great.

sarora2073 commented 6 years ago

This forum is intended for making feature requests / bug fixes etc. to the Angular Material2 team, not for support etc. The question from kdubious is better reserved for forums like stackoverflow.

kdubious commented 6 years ago

@sarora2073 I'm not asking for support, but I see now that my post wasn't clear. Instead, I'm trying to add a seemingly standard use case to your request to remove/eliminate the displayWith as it is currently used.

The current implementation of Autocomplete + displayWith doesn't handle the typical ComboBox setup. I'd expect it to allow a user to pick a foreign key, where they see the name and the field stores the id. This first part works. But, if you then try to filter as you type, the value passed to the component is no longer the text they are typing.

I'm not sure how best to fix this. However, I'm willing to weigh in as needed.

sarora2073 commented 6 years ago

It's supported today and you should be able to find sample working implementations elsewhere.

sarora2073 commented 6 years ago

Update: i've got the scenario I was trying to support (matAutoComplete with id as value etc.) working for now as you can see here

Defer to angular team on whether to close out this ticket. i.e. if this solution is sufficient, or if they still want to revise the displayWith directive. My personal opinion after wrestling with all the above discussion on the 2 scenarios (mine and the one detailed above by @willshowell) is that both scenarios should be supported easily without having to implement the "solution" in the stackblitz. e.g. a new directive (used in lieu of displayWith) that you can set to toggle the behavior as desired e.g.

display="value" OR display="viewValue" (to show innerText)

Again, a major motivation is that when using dynamic forms, where each control is it's own component, the displayWith function doesn't help much.

So, for example, if I have dynamically built registration form, the displayWith function needs to reference a method in my registration.component.ts file, but instead I'm limited to invoking a displayWith function in the component that builds the input control. This limitation reveals the antipattern / inversion of control issue here, when we start to depend on binding functions to directives, since it's the calling code (registration.component.ts) that usually needs to invoke the bound function. Hopefully that makes sense? I use material2 extensively and displayWith is the only instance I've come across with this anti-pattern.

mrmokwa commented 5 years ago

Any updates about this? Like @kdubious said, it's a fairly common use-case for a functional autocomplete.

michahell commented 4 years ago

Any update on this? I'm running into the same problem. Basically, I am using only values for my model and the displayWith callback is forcing me to make my model look like this: [{ display: 'string', value: 1 }, { display: 'string', value: 1 }] so I can take value.display inside the displayWith callback.

There is no way to bind a component's context to this function. Neither does it look like MatAutocomplete exposes the string that is displayed as 'triggervalue' to sidestep displayWith and hardcode it by getting a reference to MatAutocomplete and setting it directly when updating the / a model.

Using simple currying like explained here: https://github.com/angular/components/issues/4863#issuecomment-496570865 does seem to work but only if the form model is single value | value[]

angular-robot[bot] commented 2 years ago

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

angular-robot[bot] commented 2 years ago

Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.

We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.

You can find more details about the feature request process in our documentation.

crainsaw commented 11 months ago

Is there any chance that the vote will be started again for this issue? In our team we decided against this component because of the need to have this extra method which really makes the code ugly since we need to call our pipes on the display value in the component.ts (in the displayWith Function) which feels wrong. Additionally we still need to apply the same pipes in the template within the MatOption because the value is displayed there as well. So this is a duplication which needs to be kept in sync.

mrmokwa commented 11 months ago

In our team we did the same as you @crainsaw, we decided aganist to use it. Since it got really ugly for us to maintain. Don't see a bright future for this issue.