basic search functionality from probe search service #594
Conversation
There was a problem hiding this comment.
Thanks for filing @spasovski. I left some comments inline to get you started. They boil down to:
(1) let's filter out events from the postgrest query
(2) fix the ilike statement
(3) move the search API stuff into api.js as we discussed
(4) the probe type labels should be right-aligned and not identical width necessarily – is that a change from what's on dev?
(5) we need a little spinner at the top right of the search results header when we're awaiting a query to finish running. This can easily be done with a boolean that turns on / off while waiting for a search query
(6) if no results are present, we should say that instead of the "getting the probes – one second!" text
(7) the 400 error should probably be something in the search results body, not a yellow note to the right of everything
| } | ||
|
|
||
| const domain = (str, product = 'desktop') => { |
There was a problem hiding this comment.
just to note what we discussed over zoom:
- let's move this + the
probeSearchServicefunction over toapi.js - let's decompose the
domainfunction into its constituent parts so it is easier to reason about it
| } | ||
|
|
||
| const domain = (str, product = 'desktop') => { | ||
| const formattedStr = str.replace(' ', '%20'); | ||
| return `https://dev.probe-search.nonprod.dataops.mozgcp.net/probes?limit=15&select=name,definition,type&product=eq.${product}&or=(name.eq.${formattedStr},index.phfts(english).${formattedStr},description.phfts(english).${formattedStr},name.ilike.${formattedStr}%)`; |
There was a problem hiding this comment.
more follow-up from chatting:
(1) the ilike is technically not doing anything. Moving it to name.ilike.*${formattedStr}* will fix it
(2) adding &type=neq.event after the product-eq statement will filter out events, which I think we need here (we should not be returning events)
There was a problem hiding this comment.
I would suggest making the base URL for the probe-search service to be configurable in some way. At some point we may have multiple instances for dev & prod.
| on:input={handleSearchInput} | ||
| /> | ||
| {#if results.status} | ||
| <div |
There was a problem hiding this comment.
I think I would expect the error message in the search results area, not off to the side specifically. I would also expect it to be probably not yellow but kind of more in-line with our other error message.
| {#if results.length} | ||
| <div class="header header--loaded" in:fly={{ x: -5, duration: 200 }}> | ||
| <div>found {results.length} probes</div> | ||
| </div> | ||
| {:else} |
There was a problem hiding this comment.
as this is now, I get a getting the probes – one second! when there are no search results. If not results.length, then we should be saying "no search results found for "____"`
| getting the probes – one second! | ||
| </div> | ||
| </div> | ||
| <div>getting the probes – one second!</div> |
There was a problem hiding this comment.
I'm noticing that when I am waiting for the search service to return something, I'm actually not seeing any kind of spinner or anything in the title bar. We know when we're awaiting results – we could just set a flag somewhere in thsi file that is like isCurrentlyFetching and pop up the spinner on the right side of the search results header when that is true.
| on:click={() => { | ||
| page.show(`/firefox/probe/${results[focusedItem].name.toLowerCase()}/explore?${$currentQuery}`); | ||
| }} | ||
| on:mouseover={() => { focusedItem = i; }}> | ||
| <div class="name body-text--short-01">{name}</div> | ||
| <div class="probe-type label label-text--01 label--{type}">{type}</div> |
There was a problem hiding this comment.
This seems a bit off. The label sizing for scalar is the same as it is for histogram, so the padding looks a little strange. I would expect the label to be right-aligned and not all the same width.
|
Superseded by #602 |
This continues from hamilton's draft PR. Does not include updated styling considerations.