-
Notifications
You must be signed in to change notification settings - Fork 114
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
Functionality to clear input from sidenav search and icon gallery search #247
Conversation
src/components/FeatherIcon.js
Outdated
@@ -18,7 +18,7 @@ const FeatherIcon = ({ className, name, size = '1em' }) => { | |||
) : null; | |||
}; | |||
|
|||
// Icons from https://feathericons.com/ | |||
// Icons from |
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.
Looks like this comment lost the URL
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.
Ah yes that comes from my bad habit of clicking ctrl x 😅 I'll fix that
<span | ||
role="button" | ||
tabIndex={0} | ||
onKeyDown={handleKeyDown} |
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.
Is the intention for this to trigger when the button is focused?
src/components/SearchInput.js
Outdated
className={styles.clearButton} | ||
> | ||
<FeatherIcon | ||
name={value !== '' ? 'x' : 'search'} |
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.
Could probably be simplified to value ? 'search' : 'x'
since empty strings are falsy.
src/components/SearchInput.js
Outdated
onKeyDown={handleKeyDown} | ||
/> | ||
<div | ||
role="button" |
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.
Its typically not a great idea to use a div button for accessibility reasons. We should instead use a button
instead.
That being said, is there a reason you couldn't add the event handlers to the FeatherIcon
component itself?
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.
It wouldn't trigger when click on the FeatherIcon, I wasn't sure why that was happening which is why I wrapped it in a div. I will change this to a button!
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Adds functionality to the
SearchInput
component that allows to clear the input.Reviewer Notes
Type in the search bar for the icon gallery and sidenav and then a x will appear on the right-hand side of the input. Click to clear.
Related Issue(s) / Ticket(s)
Screenshot(s)
Icon gallery
Sidenav