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

fix(searchbar): clear controlled searchbar #448

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

dextertanyj
Copy link
Contributor

Problem

Support for controlled searchbars was introduced in #218. However, the clear button does not trigger the onChange function and only supports uncontrolled inputs.

Closes #446

Solution

Check if the searchbar is a controlled input by checking for the presence of onChange. If it is a controlled input, call onChange with the appropriate value.

Tests

  • Includes new story for controlled searchbar input.

@dextertanyj dextertanyj requested a review from karrui July 27, 2023 05:21
@netlify
Copy link

netlify bot commented Jul 27, 2023

Deploy Preview for objective-bell-0ffbfb canceled.

Name Link
🔨 Latest commit b7a710e
🔍 Latest deploy log https://app.netlify.com/sites/objective-bell-0ffbfb/deploys/64c77d4c69c5d800085d01a9

@karrui
Copy link
Collaborator

karrui commented Jul 27, 2023

FYI: @seaerchin

@seaerchin
Copy link

we pass onSearch only cos it's uncontrolled so idt this affects us! thx for letting me know sir @karrui

if (onChange) {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
onChange({ target: { value: '' } })

Choose a reason for hiding this comment

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

if this is controlled, why is it always firing with '' ah? just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since its controlled, we need to use onChange to clear the external state when the clear button is pressed so we send ''. For all other purposes, the onChange function is just passed to the Input component which will handle everything else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also use the useControllableState hook to convert all uncontrolled/controlled props passed into the component into a controlled component? Then we can handle the current value much better.

Copy link
Collaborator

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm

@karrui karrui merged commit 5b5dc8c into main Aug 7, 2023
11 checks passed
@karrui karrui deleted the fix/clear-controlled-searchbar branch August 7, 2023 09:16
@karrui karrui mentioned this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Searchbar clear button doesn't trigger changes in input
3 participants