Expose filter values on useFilterBarContext. #3861
Conversation
🦋 Changeset detectedLatest commit: 7d0ece2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| */ | ||
| describe("FilterBarContext", () => { | ||
| describe("useFilterBarContext", () => { | ||
| it("getActiveFilterValues returns expected values", async () => { |
There was a problem hiding this comment.
I know I had previously agreed to making the test here, but I wonder if we could make a simple scenario similar to the user flow you are building within FilterBar.spec.tsx. I know the file is long, but it's fine in the meantime until we figure out a convention on how to split it.
- Create 3 single FilterSelects (eg. Department, Manager, Job Title)
- Choose one (Dep) to house attributes of the other two (Source)
- Give one of the sources a default value
- Check the options of the Dep filter has been narrowed according to the Source from above (printing with a test-id might be faster, but otherwise opening the filter to check works)
- Update the value of the other Source
- Check the options of the Dep filter has been further narrowed
Thoughts?
There was a problem hiding this comment.
Nice. I haven't done exactly this, but I have done up a test that reflects the SR 2.0 use case. :) The test now selects options and does assertions on what arguments are passed to fetcher props.
| ) | ||
|
|
||
| // open department filter | ||
| user.click(getByText("Department")) |
There was a problem hiding this comment.
For best a11y testing practices, we always use getByRole where possible :)
There was a problem hiding this comment.
Cool beans. Makes sense to me!
| expect(queryByText("Engineering")).toBeInTheDocument() | ||
| expect(queryByText("Design")).toBeInTheDocument() |
There was a problem hiding this comment.
get will work, but also as with the other comment, use ByRole.
Also, instead of toBeInTheDocument, use isVisible (slightly different, but more explicit that it isn't just in the dom and visually hidden).
| await waitFor(() => { | ||
| expect(fetchManagerOptions.mock.lastCall).toEqual([ | ||
| { department: ["1"] }, | ||
| ]) | ||
| }) |
There was a problem hiding this comment.
Given this is moreso a behavioural test, instead of asserting against functions being called, awaiting a change in the UI is a better check.
I was a bit confused reading through at the start, jumping between seeing steps based around Department, then reading the expect being based around Manager, but having it equal to something from Department.
The actual technical implementation does not matter as much as achieving the outcome, which is what this test should be based around.
There was a problem hiding this comment.
Cool. I've updated the implementation of the jest fetcher to have a bit of logic in there to return different results based on what filter values are present. Pretty simple but it does the job. This then lets me do assertions on what's actually in the UI, rather than observe the jest spies.
| ) | ||
|
|
||
| // open department filter | ||
| user.click(getByText("Department")) |
There was a problem hiding this comment.
We should always await our user events to simulate real user activity (shown in RTL docs). Also prevents us possibly getting those "did you forget to wrap your test in act" type errors (which can also cause inaccuracies in tests).
| expect(queryByRole("option", { name: "Gotham" })).toBeVisible() | ||
| expect(queryByRole("option", { name: "Metropolis" })).toBeVisible() |
There was a problem hiding this comment.
:)
| expect(queryByRole("option", { name: "Gotham" })).toBeVisible() | |
| expect(queryByRole("option", { name: "Metropolis" })).toBeVisible() | |
| expect(getByRole("option", { name: "Gotham" })).toBeVisible() | |
| expect(getByRole("option", { name: "Metropolis" })).toBeVisible() |
| expect(queryByRole("option", { name: "Metropolis" })).toBeVisible() | ||
| }) | ||
|
|
||
| // select filter option |
There was a problem hiding this comment.
These comments are nice, but actually unnecessary since the code is self-documenting (except maybe the close filter one, that one isn't quite so clear reading just the code) :)
| // assert only Batman is shown | ||
| await waitFor(() => { | ||
| expect(queryByRole("option", { name: "Batman" })).toBeVisible() | ||
| expect(queryByRole("option", { name: "Superman" })).toBeNull() |
There was a problem hiding this comment.
| expect(queryByRole("option", { name: "Superman" })).toBeNull() | |
| expect(queryByRole("option", { name: "Superman" })).not.toBeInTheDocument() |
There was a problem hiding this comment.
I'm reasonably sure this won't work, since queryByRole will return null since it's not in the document anywhere. Subsequently, trying to assert that null is 'not in the document' throws an error.
There was a problem hiding this comment.
All my other tests where I write it begs to differ 😉 It's probably doing a similar check under the hood, but it's just more readable this way :)
| return Promise.resolve([ | ||
| { value: "1", label: "Gotham" }, | ||
| { value: "2", label: "Metropolis" }, | ||
| ]) |
There was a problem hiding this comment.
It will be clearer if we have the values be strings as opposed to numbers (eg. value: "gotham", label: "Gotham").
There was a problem hiding this comment.
Sounds good. I was thinking this too, but got lazy. 😩
| const isSupermanInFilterValue = Object.values(filterValues).some( | ||
| vals => vals.indexOf("3") > -1 | ||
| ) | ||
| const isBatmanInFilterValue = Object.values(filterValues).some( | ||
| vals => vals.indexOf("4") > -1 | ||
| ) |
There was a problem hiding this comment.
Slightly confused what is meant to be happening here. In a more real situation, you wouldn't be checking that any key has a particular value right? You'd check for a specific key to have a specific value, in this case filterValues.hero.includes("superman").
There was a problem hiding this comment.
This is a far nicer way to do what my late afternoon brain concocted. Very cool. Updated.
There was a problem hiding this comment.
Overall the test case is looking good! Just some clean up (removing comments for self-documented code, and changing the assertion for the non-existent elements).
Also just remembered, but could you just briefly also mention in FilterBar.mdx that this exists? Probably just where the existing example for the dependent options are :)
Why
KDS-1606.
As part of the SR2 project, we need to be across what values are currently selected in the filter bar so we know how to best create filter options. Given the context already has partial support for this (via
getFilterState(value)), it makes sense to extend this so we can get all values.What
The change is pretty simple - it's just exposing some state which we already have. What was trickier to write was the test coverage. Given there is no UI changing as a result of this change, nor should there be, I've created a new spec file for the filter bar context. This spec file is there to give us confidence/coverage on functionality that the context provides that can't be otherwise captured at this point in time. In this case, I'm calling a method on the context and rendering that result, which I can then do an assertion on. EZ. If there's a better way I should be doing this, lemme know. :)