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

DOP-4333: Limit search fetches to SearchResults component #1004

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Conversation

rayangler
Copy link
Collaborator

@rayangler rayangler commented Feb 7, 2024

Stories/Links:

DOP-4333

Current Behavior:

Open the network tab of the browser's dev tools, filter by "search-transport" and you should see requests for them, even on pages that aren't the search page. The search page is expected to have them.

Server prod
Landing prod - homepage
Landing prod - search
Landing staging - search (facets enabled)

Staging Links:

Open the network tab of the browser's dev tools, filter by "search-transport" and you should see requests for them only on the search pages, and no requests on non-search pages. Ideally, this PR does not change the search page's current behavior.

Server staging
Landing staging - homepage
Landing staging - search (no facets)
Landing staging - search (with facets)

Notes:

  • Main change is that a new SearchWrapper component was created to wrap SearchResults so that we can move the SearchContextProvider away from RootProvider. This will allow the existing SearchResults component to pull in search data from context without any major refactoring.
  • Also removed the fetch request for search data from the Header component. This was once used for contextual search across different docs sites, but since this functionality was removed from DOP-4105, it should be safe to remove now without any negative regression. We should consider re-adding if contextual search needs to be re-enabled.

@rayangler rayangler marked this pull request as ready for review February 8, 2024 17:57
Copy link
Collaborator

@seungpark seungpark left a comment

Choose a reason for hiding this comment

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

thanks for the explanation for contextual search! this LGTM

@rayangler rayangler merged commit ae81309 into main Feb 9, 2024
4 checks passed
@rayangler rayangler deleted the DOP-4333 branch February 9, 2024 17:24
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