AbdullahChauhan / custom-dropdown

Custom dropdown widget allows to add highly customizable dropdown widget in your projects. Features includes Search on list data, Network search, Multi-selection and many more.
https://pub.dev/packages/animated_custom_dropdown
BSD 3-Clause "New" or "Revised" License
158 stars 73 forks source link

Added delay before future request function is executed #19

Closed JesusHdez960717 closed 1 year ago

JesusHdez960717 commented 1 year ago

In the futureRequest of the CustomDropdown.searchRequest, I get data from an API (google places with places_service), but every time the user types a letter, the function is executed and an api call is made, causing an overfetching of the service. The simplest solution (in my opinion) would be to execute the futureRequest after a certain period of time, period of time that restarts if you type another letter.

The implementation is basically adding the Duration: futureRequestDelay property as part of the CustomDropdown.searchRequest constructor, this property is not required and defaults to no delay. Finally, before executing the search function, the time designated in this variable is waited. If another letter is typed during the waiting period, the waiting time is restarted.

AbdullahChauhan commented 1 year ago

Hi @JesusHdez960717, I hope you're doing well.

Thanks for contributing to my package. I've got your point for this PR and that's actually a good point but we need to address two issues first before merging this:

I hope you get these points. Do let me know your thoughts on these!

JesusHdez960717 commented 1 year ago

Hello @AbdullahChauhan, It is a pleasure for me to be able to contribute to this package, It's great. Regarding the issues:

AbdullahChauhan commented 1 year ago

I agree with your explanation and also I think loading view should make the job done for my both points. So if you could just make the loading view available, then we'll merge this. Thanks!

JesusHdez960717 commented 1 year ago

Commit done. Now the loading is visible from the first key stroke, even on delay, it's visible.

AbdullahChauhan commented 1 year ago

Hi @JesusHdez960717 I've checked the updated implementation and all looks good now. Appreciated your work 👍 For slight improvement, I just refactored some code to ignore timer execution if futureRequestDelay not provided via constructor API. Can you kindly take a look and test this on your project (if you've any working scenario present)? And do let me know if all works well. Thanks!

JesusHdez960717 commented 1 year ago

Perfect refactor, super clean solution. For my, it's perfect. :+1:

JesusHdez960717 commented 1 year ago

btw, i have another possible update, but this time will be a breaking change for the package, and I don't know if it's a good idea. It is basically the possibility of working the widget with a list of Objects of type T, instead of with a string. In my particular case, if the search is done on something more complex such as a list of objects, as in the case of google places, the filter has a string as input, but as output a list of Places, and I would like the plugin will work with a List. The solution I used was to map the names of the places to the object, and so I can filter them, but it seems to me a much more generic solution if you could work with generic objects and not just restricted to strings. If you think the idea makes some sense we can create an issue to discuss it in more detail, and if something serious is reached, you could implement it.

AbdullahChauhan commented 1 year ago

btw, i have another possible update, but this time will be a breaking change for the package, and I don't know if it's a good idea. It is basically the possibility of working the widget with a list of Objects of type T, instead of with a string. In my particular case, if the search is done on something more complex such as a list of objects, as in the case of google places, the filter has a string as input, but as output a list of Places, and I would like the plugin will work with a List. The solution I used was to map the names of the places to the object, and so I can filter them, but it seems to me a much more generic solution if you could work with generic objects and not just restricted to strings. If you think the idea makes some sense we can create an issue to discuss it in more detail, and if something serious is reached, you could implement it.

Yup, you are right about this and this would have been a big plus for this package. Actually, this has been in my plans for longer time but couldn't able to get time for this. Hopefully, I'll implement this in one of next release!

JesusHdez960717 commented 1 year ago

I will take a look in this days about the List change, and if it's possible, I will try to do an initial version. Maybe between the two of as could make some progress. In any case, I'm happy to be able to collaborate with this package and you @AbdullahChauhan. Thanks for all the attention. Hope this PR will be merged soon. Best regards

AbdullahChauhan commented 1 year ago

Yeah, that would be good if you initiate this. I highly appreciate your interest and work for this package. Will merge this soon!