-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Implement Search in Peregrine #1078
Conversation
This pull request is automatically deployed with Now. |
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.
Looked over the recent changes, I think it looks great.
I resolved all of my conversations that warranted it, so there's only a few outstanding questions / issues that folks had.
Feel free to resolve conversations, if I had a question on whether something had been resolved or not I left it open to be safe.
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 clarifications. I understand your listener hooks better now. The only reason I'm not approving this is followups on two comments for small renames/reorgs. Everything else looks great.
useEffect(() => { | ||
setValue(value); | ||
}, [setValue, value]); | ||
}; |
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.
@jimbo What do you think of changing this hook's name?
}); | ||
|
||
return instance; | ||
}; |
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.
@jimbo Is it safe to delete this utility since it's in Peregrine now?
Regarding the name of Regarding |
@jimbo How about |
Description
Primary changes
SearchBar
and its subcomponents to use hooksperegrine
(listed below)Other changes
informed
from1.10.12
to2.1.13
informed
react-apollo
from2.4.1
to2.5.3
react-apollo
as a peer dependency ofperegrine
react-test-renderer
from16.8.3
to16.8.6
react-test-renderer
as a dev dependency ofperegrine
TestRenderer.create()
withact()
mergeClasses
fromclassify
New custom hooks
useApolloContext
useDocumentListener
useDropdown
useQuery
useQueryResult
useValueFromSearchParams
setValue
) with the value of a search paramFile overview
Related Issue
Closes #1062.
Motivation and Context
We're starting to move some Venia components to Peregrine, and in the process we'll be extracting as much of their functionality as possible into hooks. Peregrine currently doesn't share a lot of dependencies with Venia, but it's going to have to pick some of them up as a result of this work.
Here's the big question: given a Venia component, should we extract all of its functionality (i.e., everything before the
return
statement) into a custom hook and move that hook into Peregrine? If we do, end users can use the hook to render fully custom structure, similar to render props. Just like render props, though, the API of such a hook (what inputs it expects, and what output it returns) would be unique, so you'd have to consult documentation or source code to use it. Also, naming such a hook can be challenging.