[AutoComplete] Add key(text) support for dataSource and more general item componenta support

[yongxu]

Rewrite some code to replace my initial PR, improved readability.
Broader item value support.
Clearer code.
Support for case #3137

[mbrookes]

Hi @yongxu - thanks for doing this.

Before we review the rewrite of the other PR - what is the purpose of the sort prop? If the data is sorted are the results not also? The example doesn't work as an autocomplete - it shows all results no matter what is typed, and in any case I think this functionality should be in a separate PR.I would have also suggested an issue to discuss the use case before you did the work. 😄

[nathanmarks]

@mbrookes I think we should move away from baking in application logic into the core autocomplete component. As a user I want access to a controlled component that isn't held back in any way by a maze of internal logic. Any helpers for searching/sorting data should be plugged in exactly the same way that you would plug in your own logic, imo.

[alitaheri]

Please add the signature for this too 😅

[mbrookes]

Lets figure out if this is a useful feature first. 😄

[alitaheri]

Yeah was thinking the same actually. 😅

[oliviertassinari]

I think we should move away from baking in complex logic into the core autocomplete component.

I completly agree.

[yongxu]

@mbrookes @nathanmarks @alitaheri @oliviertassinari ,
Thanks for the feedback, here are the reason why I introduced sort:
It was not possible to sort the results after the filtered result. Doing sorting after applying filter can be much faster than sorting first, which is quite a waste.
I also remembered there were people asking for this feature, can't remember the details exactly, but this can be quite useful.
@mbrookes,

The example doesn't work as an autocomplete - it shows all results no matter what is typed

The result was sorted, but not filtered. I made a mistake for not noticing the fruit list was already sorted.

[oliviertassinari]

Doing sorting after applying filter can be much faster than sorting first, which is quite a waste.

We can't disagree here. But that's not what is at stake.
Our concern is that the data handling logic can't scale to all the needs people might have. We feel like it's only adding more complexity without real benefit. I still strongly believe that we should aim to remove the data handling logic from the core of the AutoComplete.

Wouldn't it be better if we had a controlled AutoComplete component and a helper to apply some simple data logic? (sort, filter, map)
Then, if people have custom needs, they can still use the core component without the overhead.

[nathanmarks]

@oliviertassinari

I agree:

I think we should move away from baking in application logic into the core autocomplete component. As a user I want access to a controlled component that isn't held back in any way by a maze of internal logic. Any helpers for searching/sorting data should be plugged in exactly the same way that you would plug in your own logic, imo.

[yongxu]

Wouldn't it be better if we had a controlled AutoComplete component and a helper to apply some simple data logic? (sort, filter, map)

This enhancement is exactly intended for this kind of logic. As far as I know, there isn't anyway to manipulate the data or change the order(sorting) after the filter process.

Currently, we have to presort all the data before send it to dataSource if we want sorted result. Although I am not sure if this solution is the best, but there should be at least one way to do it.
@oliviertassinari @nathanmarks @mbrookes

[oliviertassinari]

As far as I know, there isn't anyway to manipulate the data or change the order(sorting) after the filter process.

Yes you can, this is how I'm using the component, I can't rely on the internal logic.
You just need to update the dataSource when a onUpdateInput or a onNewRequest event trigger.

[nathanmarks]

@yongxu As Olivier said, the direction we're moving towards would mean helpers such as this belong in optional utilities to plug into a less opinionated autocomplete. Right now, adding more built-in state logic does not help us move towards that goal.

I'd recommend filtering the data yourself and pass it to the component as @oliviertassinari outlined in the last post.

[nathanmarks]

@oliviertassinari FYI, I think @yongxu is feeling rather confused as he had the same changes accepted 9 days ago by @mbrookes in #3182

I'm sorry for any misunderstanding @yongxu -- this likely would have been raised in the 2nd review before the PR was merged.

[yongxu]

@oliviertassinari @mbrookes
I removed sort, which isn't the main purpose of this PR. Let's get this merged so we can move forward.

@nathanmarks I wasn't confused, but uuuumm...

Thanks for all the thought BTW 👍

[oliviertassinari]

I have deprecated this property. Can you use openOnFocus instead?

[oliviertassinari]

innerDivStyle={styles.innerDiv}

[yongxu]

updated :)

[nathanmarks]

@yongxu Can you edit the title and the description to reflect what the PR does now? 👍

And the commit message can match the title

[oliviertassinari]

@yongxu Thanks 👍.

[yongxu]

Thanks @oliviertassinari , I will make sure to get it right next time 👍