AbdulRahmanAlHamali / flutter_typeahead

A TypeAhead widget for Flutter, where you can show suggestions to users as they type
BSD 2-Clause "Simplified" License
831 stars 349 forks source link

feat: add itemBorderRadius and wrap itemBuilder with IgnorePointer #584

Closed coder-with-a-bushido closed 3 months ago

coder-with-a-bushido commented 6 months ago

Description

Fixes https://github.com/AbdulRahmanAlHamali/flutter_typeahead/issues/585 Fixes https://github.com/AbdulRahmanAlHamali/flutter_typeahead/issues/586

This PR does 2 things:

  1. Adds a new property itemBorderRadius which will be applied on the default itemBuilder that is wrapped on the itemBuilder passed by the user.
  2. Wraps the itemBuilder passed by the user with an IgnorePointer so that the interaction behaviour (ex. mouse hover) of the wrapping default itemBuilder is not affected. This shouldn't really change much for the user as any onTap or onPress event from the passed itemBuilder is being ignored already due to the default itemBuilder.

Checklist

Please check if your PR fulfils the following requirements:

clragon commented 6 months ago

Thank you for your work.

The reason issue #585 occurs is because the user has no direct access to the injected inkwell, which we provide for convenience. However, we do not wish to introduce hyper-specific properties like border radius because as I am sure you have seen, passing them through is tedious. Ideally, this would be solved by moving more control to the user, and doing less predefined work. This is a fine balance and I have tried my best to get the material defaults to be amicable.

It is however already possible to achieve this behaviour without a change in the package; We can use the RawTypeAheadField and customize our field as much as we wish. We do lose out on the material defaults when doing that. The problem that arises is that we cannot both provide defaults and have control. Its a trade-off or we end up with too many parameters that seem unwieldy.

One solution to this problem could be to expose the material and cupertino defaults. We could also bite the bullet on unwieldy parameters and allow more customisation that way. However, I would rather go the exporting defaults path.

For that reason, I do not wish to add a parameter like border radius directly to these components.

The reason for issue #586 is that ListTiles will override the cursor that our surrounding inkwell sets, when they have no ontap method of their own.

This, too, is a hallmark of the way we provide defaults, and could be solved by more freedom for the developer but less convenience.

For example, the Autocomplete widget that is inbuilt into the framework puts the entire burden of implementation onto the developer themselves, with which they also inherit the freedom of customisation. Instead of adding a IgnorePointer to the wrapper, which will prevent all children of these tiles from ever receiving pointer events, which I think might be considered undesirable, perhaps this too could be solved by making the defaults less built into the widgets we provide in the library.

For that reason, I dont wish to add a IgnorePointer to the default wrappers.

coder-with-a-bushido commented 5 months ago

Do you think at least exposing TypeAheadMaterialDefaults and the cupertino one sounds good? Otherwise the user has to manually copy paste builder, errorBuilder and loadingBuilder with RawTypeAheadField.

clragon commented 5 months ago

Yes, I think exposing those would be an idea. It could be done together with #580

clragon commented 3 months ago

Closing this PR, as we'll focus on implementing the solution as discussed above in a new PR instead. Thank you for your contribution.