Skip to content
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

Restore AddSearch Experiences (Mostly) #8665

Closed
wants to merge 2 commits into from

Conversation

rachelwhitton
Copy link
Member

@rachelwhitton rachelwhitton commented Aug 31, 2023

Replaces #8664

If we want to keep debugging I'm down but also not sure if we want to sink much more time in this specific 3rd party search solution so I'm proposing this as an alternative to #8664

Effect

Fixes the busted AddSearch experience across /search and 404 pages

Summary

Building off of some strong progress achieved by @stevector which replaced the stale AddSearch scripting with up to date installation techniques, this PR:

  • Fixes up /search, by binding the existing search form via CSS selector to Steve's new Search UI instance, and includes a first pass style alignment for the results, also fixes pagination
  • Fixes the search experience across 404 pages, by generating a ${searchpath} query paramater based on requested URL, allowing a visitor the enter the autofilled search form which then loads the results on the separate results /search page

Steve Persch and others added 2 commits August 23, 2023 17:04
For /search, bind existing search form via CSS selector, first pass style alignment for separate results page
For 404s, Generate ${searchpath} query paramater based on requested URL, then upon entering the form - load that query in the separate /search results page
@rachelwhitton rachelwhitton requested a review from a team as a code owner August 31, 2023 20:53
@guardrails
Copy link

guardrails bot commented Aug 31, 2023

⚠️ We detected 2 security issues in this pull request:

Hard-Coded Secrets (2)
Severity Details Docs
Medium Title: Hex High Entropy String
var client = new AddSearchClient('a7b957b7a8f57f4cc544c54f289611c6');
📚
Medium Title: Hex High Entropy String
var client = new AddSearchClient('a7b957b7a8f57f4cc544c54f289611c6');
📚

More info on how to fix Hard-Coded Secrets in General.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

@pantheon-decoupled
Copy link

⚡ Deployed with Pantheon Decoupled

This build was successfully deployed with Pantheon. You can track the build logs here.

👀 Preview: https://pr-8665-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link
Contributor

@stevector stevector left a comment

Choose a reason for hiding this comment

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

@rachelwhitton I added some minor inline comments. I think we should get a front-end engineer to review more closely.

@@ -2,6 +2,7 @@ import React from "react"
import { StaticQuery, graphql, Link } from "gatsby"
import './style.css';
import AddSearch from "../../components/addSearch"
import AddSearch2023 from "../../components/addSearch2023"
Copy link
Contributor

Choose a reason for hiding this comment

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

I went with this 2023 file name while doing side by side comparison with the pre-existing AddSearch. Can we just use AddSearch now?

@@ -2,62 +2,100 @@ import React from "react"
import Layout from "../layout/layout"
import SEO from "../layout/seo"
import SVG404 from "../../source/images/404_dark.svg"
import { Link, graphql } from "gatsby"
Copy link
Contributor

Choose a reason for hiding this comment

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

@rachelwhitton is it accurate that the 404 behavior you wanted to get working just isn't working? If so, I'd rather not add non-working code to this file

@@ -6,7 +6,7 @@ import SEO from "../layout/seo"

class Search extends React.Component {
componentDidMount() { //On page load...

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

@rachelwhitton please remove the commented out code

@@ -33,7 +33,74 @@ class Search extends React.Component {
script.setAttribute("defer", true)

document.body.appendChild(script)
*/
const script2 = document.createElement("script") // Loads the Addsearch JS blob from them
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pick more meaningful variable names?

"src",
`https://cdn.jsdelivr.net/npm/[email protected]/dist/addsearch-search-ui.min.js`
)
// script3.setAttribute("defer", true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line please if we're not going to use it

)
link.setAttribute("rel", 'stylesheet')

//alert('mount up');
Copy link
Contributor

Choose a reason for hiding this comment

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

remove stray debugging code


const link = document.createElement("script") // Loads the Addsearch JS blob from them
link.setAttribute(
"href",
Copy link
Contributor

Choose a reason for hiding this comment

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

did this loading of the style sheet end up working?

}
addSearchStuff() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you chose a more meaningful method name? I wrote this name before I knew what it would do.

}
addSearchStuff() {
//alert('add it');
Copy link
Contributor

Choose a reason for hiding this comment

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

remove stray debugging code please



var searchui_conf = {
//searchResultsPageUrl: 'search'
Copy link
Contributor

Choose a reason for hiding this comment

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

remove unused code please


class AddSearch extends React.Component {
componentDidMount() {
const resultPage = "/hi-rachel"
Copy link
Contributor

Choose a reason for hiding this comment

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

hi-rachel probably isn't what we want here 😆


// Add components
searchui.searchField({
containerId: 'searchfield',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code is meant to add a form that is different from the form in the header. But looking at https://pr-8665-documentation.appa.pantheon.site/search/, I only see the form in the header

@@ -612,3 +612,58 @@ a.release-update::before {
font-family: 'Glyphicons Regular';
src: url(/fonts/glyphicons/glyphicons-regular-v192.woff2) format('woff2');
}
.addsearch-searchresults h3{
color: #000 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's been many years since my regular CSS writing days, but I think !important is still only for use as a last resort. Can you make the selector more specific?

@rachelwhitton rachelwhitton marked this pull request as draft September 29, 2023 19:44
@rachelwhitton
Copy link
Member Author

Closing in favor of #8707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants