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

support boolean facets #94

Merged
merged 1 commit into from
Apr 25, 2024
Merged

support boolean facets #94

merged 1 commit into from
Apr 25, 2024

Conversation

abeglova
Copy link
Contributor

@abeglova abeglova commented Apr 22, 2024

What are the relevant tickets?

Description (What does it do?)

This updates course-search-utils to support boolean facets

How can this be tested?

Test with mitodl/mit-learn#802

@abeglova abeglova changed the title Ab/boolean facets support boolean facets Apr 23, 2024
@abeglova abeglova marked this pull request as ready for review April 23, 2024 12:41
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

Two requested changes:

  • the extra tests
  • the type change for setParamValue

initial: "?department=6",
expected: new URLSearchParams("?department=6&resource_type=program")
}
])("setSearchParams sets string params", ({ initial, expected }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it would be worth adding a few more tests, particularly array values:

  test.each([
    {
      initial:  "?resource_type=course&resource_type=program&department=6",
      value:    "program",
      expected: new URLSearchParams("?department=6&resource_type=program")
    },
    {
      initial:  "?department=6",
      value:    "program",
      expected: new URLSearchParams("?department=6&resource_type=program")
    },
    {
      initial:  "?department=6",
      value:    ["program", "video"],
      expected: new URLSearchParams(
        "?department=6&resource_type=program&resource_type=video"
      )
    },
    {
      initial:  "?department=6&resource_type=program",
      value:    [],
      expected: new URLSearchParams("?department=6")
    }
  ])("setSearchParams sets string params", ({ initial, expected, value }) => {
    const { result, searchParams } = setup({ initial })
    act(() => {
      result.current.setParamValue("resource_type", value)
    })
    expect(searchParams.current).toEqual(expected)
  })

@@ -58,6 +58,9 @@ interface UseValidatedSearchParamsResult<ReqParams> {
* - if `checked=false`, value is REMOVED from parameter list.
*/
toggleParamValue: (name: string, rawValue: string, checked: boolean) => void

setParamValue: (name: string, rawValue: string) => void
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be

Suggested change
setParamValue: (name: string, rawValue: string) => void
setParamValue: (name: string, rawValue: string | string[]) => void

right? That allows setting department=6&department=8, and also is consistent with the type below.

@abeglova abeglova force-pushed the ab/boolean-facets branch from 9511e6f to 1ce3aa1 Compare April 24, 2024 15:10
@abeglova abeglova force-pushed the ab/boolean-facets branch from 918b894 to 6d1cb40 Compare April 25, 2024 15:48
@abeglova abeglova merged commit 4a6974a into main Apr 25, 2024
4 checks passed
@odlbot odlbot mentioned this pull request Apr 25, 2024
5 tasks
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.

2 participants