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

Fix separate search results page #8707

Merged
merged 4 commits into from
Oct 5, 2023
Merged

Fix separate search results page #8707

merged 4 commits into from
Oct 5, 2023

Conversation

rachelwhitton
Copy link
Member

@rachelwhitton rachelwhitton commented Sep 27, 2023

replace #8665

The search field after the PDS Theme was implemented has a better experience than the (at the time) current production theme, so I switched efforts to this PR and I'm getting expected behaviors now on local and the preview env (in #8665 separate search results page worked locally but not on the preview)

Expected Search Experience:

  • When you are using the search widget (everywhere except /search) and you hit enter, you are now directed to the separate search results page with the query passed in and results shown on page load
  • When you are using the separate search results page (/search) the results should upate as you type, hitting enter in this search field on this page should not load a new page but it should update the search URL paramater in the address bar
  • Outstanding: Style separate search results

@rachelwhitton rachelwhitton requested a review from a team as a code owner September 27, 2023 21:42
@guardrails
Copy link

guardrails bot commented Sep 27, 2023

⚠️ We detected 1 security issue in this pull request:

Hard-Coded Secrets (1)
Severity Details Docs
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.

@rachelwhitton rachelwhitton changed the base branch from PDS-theme to main September 29, 2023 19:36
@rachelwhitton rachelwhitton marked this pull request as draft September 29, 2023 19:44
@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-8707-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

@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-8707-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

@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-8707-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Styles for if/when number-of-results becomes an h2
@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-8707-documentation.appa.pantheon.site
🛠️ Manage in Pantheon: https://dashboard.pantheon.io/site/2b30153f-e8b1-4427-b076-6109e704ba5d/overview

Copy link
Member

@mel-miller mel-miller left a comment

Choose a reason for hiding this comment

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

It works!

The search results page is themed. I'm going to also theme the dropdown search results, but I'll do that in a separate PR so as to not hold this one up.

I'm not sure how much control you have over the markup from addsearch, but if possible the "number-of-results" should probably be an h2. Alternatively, the search hit titles could be h2 instead of h3. Either solution would work for proper heading levels on this page for accessibility purposes. I wouldn't hold up the whole thing for this, as it can always be fixed later, but I thought I'd mention it in case it was an easy change.

@rachelwhitton rachelwhitton marked this pull request as ready for review October 3, 2023 16:40
@stevector
Copy link
Contributor

I'm not sure how much control you have over the markup from addsearch, but if possible the "number-of-results" should probably be an h2. Alternatively, the search hit titles could be h2 instead of h3. Either solution would work for proper heading levels on this page for accessibility purposes. I wouldn't hold up the whole thing for this, as it can always be fixed later, but I thought I'd mention it in case it was an easy change.

I'd prefer we handle that in a follow up.

@stevector
Copy link
Contributor

Also, thanks @mel-miller! I'm passing this along internally and hope to have it merged soon.

@stevector stevector merged commit 8bfa249 into main Oct 5, 2023
6 checks passed
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.

3 participants