-
Notifications
You must be signed in to change notification settings - Fork 16
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
Search: wrap summary data to Search component #104
Search: wrap summary data to Search component #104
Conversation
src/components/Search/Search.tsx
Outdated
let summary = {}; | ||
if (response) { | ||
summary = { | ||
...mapToObject(response.getQueryValues()), |
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.
The following two lines have the potential to override each other, q
in Query values will get overridden by q
in responseValues if autocomplete is running. It might be worth having a queryValues
, and responseValues
key in the SummaryInterface
.
...mapToObject(response.getQueryValues()),
...mapToObject(response.getValues()),
src/components/Search/Search.tsx
Outdated
export interface ResponseInterface { | ||
reads: number; | ||
totalResults: number; | ||
time: number; |
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.
@benhinchley I've updated the code as your suggestion and also explicitly made the keys of queryValues
,.. visible rather than using [key: string]: number | string
. Not sure it's ok to you because I guess those properties would be permanent.
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.
Hey @zlatanpham the values for a query, can be anything as a user can customise the variables needed for their pipeline. So the correct type for these objects is actually { [k:string]: string | number }
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.
updated!
bbc3072
to
a844c55
Compare
WHAT:
I am using the
Search
component to build the interface for #102 but the UI requires data liketotalResults
,q.original
, etc and I have to useConsumer
to get these data.WHY:
It's cumbersome to write nested render-prop components since we can do an update to use just
Search
only.HOW:
Get all the needed data from
pipeline
methods likegetResults
orgetResponse
and inject those tosummary
prop and pass to render prop function.