-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(search): add onClear callback prop to Search/DataTableSearch #9737
feat(search): add onClear callback prop to Search/DataTableSearch #9737
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: f8807c6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/615f6b683bfa400007b1cda3 😎 Browse the preview: https://deploy-preview-9737--carbon-react-next.netlify.app/ |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: a723820 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/614df9d05c8ad90007ea3f0b 😎 Browse the preview: https://deploy-preview-9737--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: a723820 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/614df9d0aaf89500075f5ff0 😎 Browse the preview: https://deploy-preview-9737--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: b51b682 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/614dfca83425540007314a63 😎 Browse the preview: https://deploy-preview-9737--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: b51b682 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/614dfca88b0d0b00081e0839 😎 Browse the preview: https://deploy-preview-9737--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: f8807c6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/615f6b685a653700078fc688 😎 Browse the preview: https://deploy-preview-9737--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: f8807c6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/615f6b6856468a00071e6116 😎 Browse the preview: https://deploy-preview-9737--carbon-components-react.netlify.app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @kevinsperrine ! 🎉
This addition makes sense to me. Another option would be to modify the signature of the onChange
to include a new isClear
param or something, but based on #9641 I don't think we'd want to do that.
I see the onClear
fire in the DataTable storybook, but I don't see it firing in the Search story?
no.onClear.mov
Otherwise this looks solid 👍
@tay1orjones Thanks for the heads up. I have no idea what's going on in the deploy preview. Locally, there are onChange, onKeyDown, and the onClear handlers being fired, but only onChange in the preview. 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinsperrine I'm not sure what's going on there. Locally I see the same though; onKeyDown
and onClear
logging as expected to the actions pane. It might be something with the deploy preview environment and the fact that you're a first time contributor so some status checks had to be approved to be ran. The production storybook was/is logging things fine.
I'm good to merge this, if the storybook issue persists in production I can open a new issue to hunt it down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the contribtion!
Closes #9738
We needed a way in the
carbon-addons-iot-react
repo to only trigger search when a user hitsEnter
or blurs the input. The problem was with all changes pushed throughonChange
there's no way to tell if they backspaced to delete everything or if the "Clear search input" button was clicked. This PR adds anonClear
callback prop to distinguish between the two.Changelog
New
Testing / Reviewing
Added new tests to Search component to confirm callback is called.