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

Simplify search handling #1324

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Dec 10, 2023

In these cases, callback methods are not needed and binding directly to the field is sufficient.

Microsoft Reviewers: Open in CodeFlow

In these cases, callback methods are not needed and binding directly to the field is sufficient.
@tlmii
Copy link
Member

tlmii commented Dec 11, 2023

It looks like some of the intent in that HandleClear method got lost along the way. I think it was intended to make sure the _filter didn't get set to null (because value on FluentSearch is string?) - it's in the FluentUI Demo site too. But then the conditional got lost at some point so the current implementation doesn't make much sense.

@drewnoakes
Copy link
Member Author

@tlmii is there something you'd like to see changed here?

@drewnoakes drewnoakes requested review from vnbaaij and tlmii December 18, 2023 01:12
Copy link
Member

@tlmii tlmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Digging through the underlying code I'm not sure why that needed to be there to begin with. Maybe it was a historical artifact, but it seems clearly not necessary now.

@drewnoakes drewnoakes merged commit 4e334a5 into dotnet:main Dec 18, 2023
@drewnoakes drewnoakes deleted the simplify-search-boxes branch December 18, 2023 11:56
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants