Skip to content

feat(search)#700

Merged
kevee merged 24 commits intofeature/header-refreshfrom
feature/search
Apr 29, 2020
Merged

feat(search)#700
kevee merged 24 commits intofeature/header-refreshfrom
feature/search

Conversation

@superbiche
Copy link
Copy Markdown

@superbiche superbiche commented Apr 23, 2020

Description

Draft PR to add Algolia search (#693)

Original todo from @kevee, please add items here so we don't get lost :) :

  • Ordering by index type, i.e. show states first, then blog posts, then pages - if I type "California" I probably want to jump to California's page
  • A search results page at /search that supports queries - i.e. /search?q=stuff
  • A search box that goes on the header. Do not worry about design right now, but just make sure it works.
  • A search autocomplete - we already use Reach's combobox component on the site for the state dropdown, and I like it because of all all the accessibility features in it, but I am not wedded to that component
  • Environment-aware Algolia index (https://covid-tracking.slack.com/archives/C012DPFRP7D/p1588079930004600)
  • Target the feature/header-refresh branch

Related Issues

#693

@superbiche superbiche added enhancement New feature or request DEV labels Apr 23, 2020
@superbiche superbiche self-assigned this Apr 23, 2020
@superbiche superbiche changed the title Feature/search feat(search) Apr 23, 2020
…n query string

feat(search): add some styles & links for states/blogPosts results
@muamichali
Copy link
Copy Markdown
Contributor

I think it could be useful to scrape our own tweets and index them as well, since a lot of useful information/thoughts/explanations happen on our twitter feed.

@superbiche
Copy link
Copy Markdown
Author

@muamichali yup that's something we can work on, if someone's willing to create the app and send us the API keys on Slack, we'd be happy to give it a shot when we're done with the basic features!

Michel Tomas added 5 commits April 25, 2020 18:31
The search feature was started as a PoC using State hooks,
that was extracted to src/context/search-context,
using Context hooks for easier reuse (on /search but also in the header).

Added withSearch HOC, which wraps withLocation, to re-use
searchContext easily accross the site while retaining access to URL query.

All logic specific to search, but not to search page, moved to
the hook file.

Pages that match have their (full, for now) body rendered with
some highlighting (deserves some more emphasis to make it more visible).
If the body has no hightlighting we return its truncated value.

Finally - more like a side effect, but a nice one - the search is
triggered whenever the input value is updated. We need to check:
1. Is this going to consume our API quota too fast, and
2. Will this be, OK as the header will be an autocomplete,
and this input refreshes the result page instantly? The UX feels right
but maybe I'm just happy that it works.
@netlify
Copy link
Copy Markdown

netlify bot commented Apr 26, 2020

This is a preview version of the site.

Built with commit 7eabe66

https://deploy-preview-700--upbeat-lovelace-3e9fff.netlify.app

Copy link
Copy Markdown
Contributor

@schwartzadev schwartzadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a first pass from visually inspecting the code and chatting with @superbiche over Slack.

I didn't get a chance to run this locally to review, but I'm posting this in the interim regardless

</div>
)}
<div className={searchPageStyle.searchResults}>
{/* State results */}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could probably be done DRY-er -- we could have a component for the results and then pass the inner content to the component for:

  • state results
  • blog posts
  • pages

Plus, this should make it easier to add items like Twitter in the future!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree. Once we've done the first review pass this ought to be a flexible component!

{searchState.isFetching && (
<div>
<img
src="https://d1j8pt39hxlh3d.cloudfront.net/products/previews/RES3POBSZ353HFVPZOKR/2329_kKbUTLGEVXhuOIJqrT7QvIJFAXvEpQ3z.gif"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a placeholder for now -- just flagging it before we push

<div key={page.objectID} className={searchPageStyle.searchResult}>
<PublicationTitle>
{/* FIXME this should be handled during indexing
(avoids external Link issues) */}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky...it looks like some Links begin with '/', and others don't. We should probably standardize this across the codebase so we don't have to handle this here. Ideally someone could work some grep magic and find out where that's happening.

@kevee which is preferred?

  1. prefixing Link with /
  2. not prefixing Link with /

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we have to prefix links with / otherwise Gatsby will throw warnings.
I'm pretty confident this is an inconsistency issue with the slugs and how these are built.

I could comment out this function to get a precise list of failing links, so we can check these individually

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix them all with /

The pages here all have their slug defined in Contentful, which automatically creates the slug based on the title.

Copy link
Copy Markdown
Author

@superbiche superbiche Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevee Added getSanitizedSlug to generate slug depending on result type + handle missing starting /

@kevee
Copy link
Copy Markdown
Contributor

kevee commented Apr 27, 2020

This is going to be a feature in a larger project, so let's not merge it into master. I'll make a project branch for it to merge into tomorrow.

@kevee
Copy link
Copy Markdown
Contributor

kevee commented Apr 27, 2020

I've also made a separate admin key for both Netlify and CircleCI scoped to just index management.

@superbiche
Copy link
Copy Markdown
Author

Thanks for your feedback @kevee @schwartzadev. Fixes and adjustments coming later today!

@jasonlcrane
Copy link
Copy Markdown
Contributor

I added the autocomplete, using the Reach UI Combobox. I kept the form that Michel has been using on the page as well, for discussion's sake. I am not sure what the best approach is, but one thing I was thinking about was the possibility of ditching this autocomplete, and having the search results with snippets that Michel has displaying on the page go full screen on search (so essentially still autocomplete). There are some potential accessibility concerns there I'm sure that we would need to work around.

Copy link
Copy Markdown
Author

@superbiche superbiche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a quick review, some issues to fix but nothing shocking 👍


let searchEvent
const getSlug = item => {
if (!item.publishDate) {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's risky to rely on item.publishDate, imo adding a type argument to the arrow function would be safer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right, that's brittle. The type is not known when getSlug is called, though. We could refactor it to loop over those hits arrays separately and set the type there, I think. That could get a little messy as well, but I'll see if I can come up with something that will work.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo looping over the results while retaining the type is the way to go, added searchResultTypes const to make it easier and gonna make result keys use keys from this constant so it should be more robust

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used your new keys and getSanitizedSlug function to improve this.

return `/blog/${item.slug}`
}

const goToResult = selectedItem => {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It amazes me Combobox doesn't allow choosing what we pass. @kevee do you know if there's a way to pass the item.objectID instead?

})

if (item && typeof window !== 'undefined') {
window.location = getSlug(item)
Copy link
Copy Markdown
Author

@superbiche superbiche Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasonlcrane check out withLocation HOC, it passes location and navigate if I remember well.
You could add navigate to the component's props export default withSearch(({ search, navigate }) => { so you can then use it to go to the destination while retaining history.
(https://reach.tech/router/api/navigate)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good call. Updated.

@schwartzadev
Copy link
Copy Markdown
Contributor

image

  • I don't think this is expected behavior...How does the autosearch prioritize results? I would expect the Data xyz pages to be the first results, not Wyoming, etc.

image

  • Design-wise, we should have a more clear indicator of what is a query and what is content (the query covid looks the same as deprecated, for example)

image

  • We should have spacing between the center bullet and the names/dates -- see the homepage for reference

Is there a reason for returning the whole page content under Pages? I would imagine something like the following would be more helpful:

Pages

Data API:

  • ...text surrounding the query from the user...
  • ...a query is provided by the user via the nav...

Another Page:

  • ...only one match for query.

  • It looks like the inline rendering is not working properly:
    image
    The sentence after Javascript should be on a new line, the text below it should not be indented.

  • % seems to be a wildcard (I'm getting all the states back, two pages, and both blog posts). If that's true, do we want to include that in production?

  • The accessibility statement renders as: We welcome your feedback on the accessibility of COVID Tracking Project Website. Please let us -- it looks like the end of the content is cut off. Maybe this has to do with the chunking processes?

@superbiche superbiche changed the base branch from master to feature/header-refresh April 28, 2020 20:22
@kevee kevee marked this pull request as ready for review April 29, 2020 15:48
@kevee kevee merged commit f7655ae into feature/header-refresh Apr 29, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/search branch April 29, 2020 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DEV enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants