-
Notifications
You must be signed in to change notification settings - Fork 81
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: Make search component extendable #2230
Conversation
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! Everything is working and passing. Great job Pearl!
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 contributing! If you want your github profile linked in our readme you can run yarn contributors:add <github user name> code
and commit the changes before you merge.
import { render } from '@testing-library/react' | ||
import { SearchField } from './SearchField' | ||
|
||
describe('SearchField component', () => { |
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.
question: I wonder if it's possible/worth the time to add a test ensuring that the new inputProps
are passed down as expected? I can go either way on it so I'll leave it up to you :)
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 review! I'll add additional test before I merge this
… pm-make-search-component-extendable
82408c0
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! Great work!
@gidjin @NamibiaTorres I added the additional test. However, I'm seeing a build issue that I can't reproduce locally, it seems to be a CircleCI environment issue. I found some context here. If either of you have seen this issue before please let me know, I'm not sure how to proceed |
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 updates!
Summary
This PR:
inputProps
toSearch
and passes it down toTextInput
Search
into two new componentsSearchField
andSearchButton
Related Issues or PRs
closes #2209
How To Test
yarn storybook
and look atComponents/Search/*
notice the search input and search buttons have been broken out into their own components. And are composed together inSearch
yarn test:coverage
and verify that all tests are passing and that the coverage threshold is being met.Screenshots (optional)
SearchField
SearchButton
Search