search

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
144 stars 66 forks source link

Preview: Add multiselect constructor #40

Closed KabaDH closed 9 months ago

KabaDH commented 10 months ago

This PR is a preview and is not yet ready for production.

This PR introduces a new constructor CustomDropdown.multiSelect() to add the ability to select multiple objects.

KabaDH commented 10 months ago

@AbdullahChauhan hello! PR is ready for the review.

AbdullahChauhan commented 10 months ago

Hey @KabaDH Thanks much for the contribution. 😊 I'll look into this and get back to you if anything to discuss. Very busy schedule these days, so this might take some time!

JesusHdez960717 commented 10 months ago

Hello @KabaDH, I think it is an extraordinary idea to have the possibility of adding a multiselect. 10/10 :+1:

But if I'm honest, I didn't like the way the development was thought out... what do I mean:

The component is currently a simple selection Dropdown, which already has too many (in my opinion) fields in the constructor, if the possibility of multi-selection is added to that, I consider that it greatly increases the usage curve of the end users. The attributes were literally doubled, because now they include more than one selection, the multiple selection ones.

I come back and repeat, I love the idea of having support for multiselection, but: it does not make more sense, for the end user, than, for example, creating another class MultiselectCustomDropdown or something similar with only the multiselection functionality. This would imply, I imagine, making the internal changes to adjust it to the two alternatives, but nothing that cannot be resolved with an enum (as was already done with the search types)

ideas???

KabaDH commented 10 months ago

Hello @KabaDH, I think it is an extraordinary idea to have the possibility of adding a multiselect. 10/10 👍

But if I'm honest, I didn't like the way the development was thought out... what do I mean:

The component is currently a simple selection Dropdown, which already has too many (in my opinion) fields in the constructor, if the possibility of multi-selection is added to that, I consider that it greatly increases the usage curve of the end users. The attributes were literally doubled, because now they include more than one selection, the multiple selection ones.

I come back and repeat, I love the idea of having support for multiselection, but: it does not make more sense, for the end user, than, for example, creating another class MultiselectCustomDropdown or something similar with only the multiselection functionality. This would imply, I imagine, making the internal changes to adjust it to the two alternatives, but nothing that cannot be resolved with an enum (as was already done with the search types)

ideas???

@JesusHdez960717 Hello, and thank you for sharing your thoughts!

As you can see in the PR, the only available fields for the end user in the new constructor are those related to multiselect or shared properties with the single selection types (mostly decoration properties). Therefore, I can't see how it can impact the end user experience. On the other hand, the available fields for all other single select constructors didn't change at all. The widget retains all of its simple single-select constructors (without any new fields) and gets one more - a simple multiselect with its own fields. And yes, to handle the widget type (single or multiselect), I used an enum.

I decided to create the new constructor instead of a new class for maximum reuse of the current code. Moreover, as you can see, I took the first step to add multiselect to other widget types (search ones, for example).

JesusHdez960717 commented 10 months ago

BTW, I belive it's an amazing feature, I just have some doubt on how to use the package after this update (in case it get finally merged).

What happens if I want to use multiselect with the futureRequest option? We would have to create another constructor: CustomDropdown.multiselectSearchRequest, right? (to a total of 4 constructors)

It is not easier to have two classes CustomDropdown and MultiselectCustomDropdown, each with its methods: search & searchRequest?

KabaDH commented 10 months ago

BTW, I belive it's an amazing feature, I just have some doubt on how to use the package after this update (in case it get finally merged).

What happens if I want to use multiselect with the futureRequest option? We would have to create another constructor: CustomDropdown.multiselectSearchRequest, right? (to a total of 4 constructors)

It is not easier to have two classes CustomDropdown and MultiselectCustomDropdown, each with its methods: search & searchRequest?

Yes, you are right. We add more constructors. And thats all. All shared properties will be maintained together. Anyway, it can be refactored later if the current model is not as convenient as it should be.

I cant see much value in adding the new class. In any case we add new constructors, but in this case, we have 1 class less to maintain.

JesusHdez960717 commented 10 months ago

You are absolutely right, I forked your repo and did some extensive testing and I loved it. Mainly because at the maintenance level, having another class would become more expensive than having another constructor.

Also, I don't think many more constructors will be added in the future, so that class shouldn't become too complex.

Thank you for having the patience to explain your point of view to me.

I consider that the new constructor should be added to the multiselect with the futureRequest and documented in detail to make the differences between the singleSelect and the multiSelect clear.

You would finally be left with 4 constructors, two for single select, 2 for multiselect. Right?

KabaDH commented 10 months ago

You are absolutely right, I forked your repo and did some extensive testing and I loved it. Mainly because at the maintenance level, having another class would become more expensive than having another constructor.

Also, I don't think many more constructors will be added in the future, so that class shouldn't become too complex.

Thank you for having the patience to explain your point of view to me.

I consider that the new constructor should be added to the multiselect with the futureRequest and documented in detail to make the differences between the singleSelect and the multiSelect clear.

You would finally be left with 4 constructors, two for single select, 2 for multiselect. Right?

You are very welcome!

I added only one constructor (multiselect) in the current PR. All the other current constructors remain unchanged (default, search, and searchRequest). So the total is 4 for now (3 single and 1 multi).

If there is a need for multiselect variations for search and searchRequest, it will be +2 constructors (total 6). And I totally agree, that all use cases should be properly documented in constructors, including updating the readme, as I did for the multiselect.

JesusHdez960717 commented 10 months ago

I always forget the default (I never use it :laughing:)

I do consider that the other constructors should be added to handle the search and request on the multiselect, everything would be symmetrical (3 and 3 for each DropdownType) and all the functionalities covered for each type.

AbdullahChauhan commented 9 months ago

Hey @KabaDH @JesusHdez960717 Just manage to get some free time now. Btw pretty good discussion above and I do agree with both of you. Let me take a look into the code and get back to you if there's anymore discussion required.