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

useResourceSearchParams and export AvailableFacets component #78

Merged
merged 5 commits into from
Mar 27, 2024

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Mar 22, 2024

What are the relevant tickets?

Description (What does it do?)

This PR:

  • removes useSearchQueryParams, which has not been used in any production code
  • adds useResourceSearchParams, useContentFileSearchParams. These hooks:
  • Exports AvailableFacets, a portion of the existing FacetDisplay component.
  • Replaces the in-repo generated client with client from https://github.com/mitodl/open-api-clients

How can this be tested?

Test by instructions in mitodl/mit-learn#675

@ChristopherChudzicki ChristopherChudzicki force-pushed the cc/search-cleanup branch 5 times, most recently from 9641412 to 512230e Compare March 25, 2024 18:11
Comment on lines +75 to +83
const useValidatedSearchParams = <ReqParams>({
searchParams,
setSearchParams,
facets = [],
validators,
onFacetsChange
}: UseValidatedSearchParamsProps<ReqParams> & {
validators: QueryParamValidators<ReqParams>
}): UseValidatedSearchParamsResult<ReqParams> => {
Copy link
Contributor Author

@ChristopherChudzicki ChristopherChudzicki Mar 26, 2024

Choose a reason for hiding this comment

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

There are some notable differences in these hooks vs the useSearchQueryParams, which these two hooks replace.

  1. "Facets" are less special: The old hook returned a params object with shape { queryText, sort, activeFacets }. Everything was a facet except queryText and sort.

    This was problematic for some UIs, e.g., this one which has (A) resource tbas and (B) a sidebar for facets. Previously, the "Clear All" button would reset the resource tab, even though it's not part of the filtering sidebar.

    In the new hooks, facets are determined by a parameter, facets, and clearAllFacets only clears the parameters that are listed as facets.

  2. Two separate hooks for for /api/v1/learning_resources_search and /api/v1/content_files_search. This makes the typescript much simpler, and seems reasonable since the APIs are separate.

  3. Uses API parameter names, not abbreviations. ("department" not "d").

@@ -47,8 +47,53 @@ const resultsWithLabels = (
return newResults
}

const AvailableFacets: React.FC<Omit<FacetDisplayProps, "clearAllFilters">> = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FacetDisplay consisted over three portions:

  1. A title, "Facets" and "Clear All" button
  2. the "Active Facets", displayed in OCW as "chips" with rounded corners
  3. a list of the "Available Facets"

In MIT Open, we do not want (2). Since (1) is simple, it can be implemented easily in MIT Open.

But (3) is worth exporting on its own.

@@ -37,6 +37,7 @@
},
"homepage": "https://github.com/mitodl/course-search-utils#readme",
"dependencies": {
"@mitodl/open-api-axios": "^2024.3.22",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally, I was not going to introduce the @mitodl/open-api-axios client in this PR. However, the client previously committed in this repo was missing the new resource_type: Video which caused some type errors in MIT Open, so I went ahead and replaced the client in this PR.

@ChristopherChudzicki ChristopherChudzicki changed the title Cc/search cleanup useResourceSearchParams and export AvailableFacets component Mar 26, 2024
@abeglova abeglova self-assigned this Mar 26, 2024
Copy link
Contributor

@abeglova abeglova left a comment

Choose a reason for hiding this comment

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

👍

@ChristopherChudzicki ChristopherChudzicki merged commit 8305091 into main Mar 27, 2024
2 checks passed
@odlbot odlbot mentioned this pull request Mar 27, 2024
2 tasks
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