Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add itemBorderRadius and wrap itemBuilder with IgnorePointer #584

Conversation

coder-with-a-bushido
Copy link

@coder-with-a-bushido coder-with-a-bushido commented May 15, 2024

Description

Fixes #585
Fixes #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:

  • I have read the Contributor Guide
  • I have filed an issue for the change
    [#Issue Number]
  • All code is formatted with dartfmt
  • All code passes analysis with no errors or warnings
  • Changes work for both Material and Cupertino
  • New code has documentation where needed
  • New code has test coverage if applicable
  • All existing tests pass
  • Example project still compiles and runs
  • Changes have been documented in CHANGELOG.md
    Using the Keep A Changelog format.
    You may use cider to help with this.
  • New code has been documented in the README.md if applicable
  • Commit messages follow the Conventional Commits format

@coder-with-a-bushido coder-with-a-bushido force-pushed the karthi/item-border-radius branch from 1ce5a79 to 1ff3814 Compare May 15, 2024 03:10
@coder-with-a-bushido coder-with-a-bushido marked this pull request as ready for review May 15, 2024 03:12
@clragon
Copy link
Collaborator

clragon commented May 26, 2024

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
Copy link
Author

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
Copy link
Collaborator

clragon commented Jun 7, 2024

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

@clragon clragon closed this Aug 16, 2024
@clragon
Copy link
Collaborator

clragon commented Aug 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants