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

[Task 2260] refactor search error page for maintainability #2434

Merged
merged 5 commits into from
Oct 10, 2024

Conversation

doug-s-nava
Copy link
Collaborator

@doug-s-nava doug-s-nava commented Oct 9, 2024

Summary

Fixes #2260

Time to review: 20 mins

Changes proposed

The goal of this change is to update the search error page to:

  • be up to date with recent changes to the search page (collapsable filters on mobile)
  • be more easily maintainable in the future, as currently the error page is set up to duplicate nearly all of the code from the main search page

To accomplish this, I:

  • created a layout for the search route containing as much shared DOM and functionality as possible
  • created a SearchResult component to abstract out functionality to be removed from the error page
  • updated the error page to be as in line with the new search page as possible

Context for reviewers

Note that I considered other options for approaching this change:

  • error boundaries: I could not get this to work with Next's server side rendering logic
  • let the layout handle shared functionality: I tried putting everything but the search result functionality into the layout file, but the layout doesn't have access to search params so I was limited in what I could do without a larger refactor

A few other ancillary changes:

  • some updates to error handling messaging and logic to make my internal debugging easier during development. These can be dropped if we don't want this scope creep
  • update to search error message component to leverage USWDS component
  • removal of unnecessary code from SearchFilters to allow it to be rendered client side

Test steps

  1. visit the search page
  2. VERIFY: functions as expected when performing search / filter / sort actions
  3. adjust the search request such that it will return an error - for example, update the "namespace" for the SearchOpportunityAPI such that the request will 404, or use an extension like tweak to mock an error response
  4. refresh
  5. VERIFY: an error state appears on the page

Additional information

Existing issues uncovered but not addressed:

  • in an error state the page does not entirely work - filter checkboxes when clicked will update query params but will not update in the UI
  • the error page is relying on the state of the page in the form of query params coming back in the error that page receives. If an error is thrown that does not contain this data, the state of the page is lost

acouch
acouch previously approved these changes Oct 10, 2024
Copy link
Collaborator

@acouch acouch left a comment

Choose a reason for hiding this comment

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

Awesome start at making things cleaner and easier to maintain.

status: number;
type: string;
}

function isValidJSON(str: string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should probably move to a util, but didn't want to cause unnecessary churn creating a new file for it today

}
}

function createBlankParsedError(): ParsedError {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it seems like there should be a better way but I think I'd need more context into our error throwing / handling patterns to take an informed approach. For now just tried to change as little as possible to keep this working

@@ -23,7 +22,6 @@ export default function SearchFilters({
category: Set<string>;
opportunityStatus: Set<string>;
}) {
unstable_setRequestLocale("en");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@doug-s-nava doug-s-nava marked this pull request as ready for review October 10, 2024 14:35
@doug-s-nava doug-s-nava changed the title Dschrashun/2260 search error page [Task 2260] refactor search error page for maintainability Oct 10, 2024
@doug-s-nava doug-s-nava merged commit ffaaa27 into main Oct 10, 2024
11 checks passed
@doug-s-nava doug-s-nava deleted the dschrashun/2260-search-error-page branch October 10, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor search page and search error page for reusability and consistency
2 participants