-
Notifications
You must be signed in to change notification settings - Fork 12
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
Fix useSearchParams to replace the url instead of appending to the existing #1219
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1219 +/- ##
=======================================
Coverage 95.03% 95.03%
=======================================
Files 320 320
Lines 9348 9353 +5
Branches 2029 2030 +1
=======================================
+ Hits 8884 8889 +5
Misses 456 456
Partials 8 8
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add tests
`); | ||
fireEvent.click(viewDetailsLink); | ||
expect(history.location.pathname).toEqual('/admin/CareTeams/308'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(history.location.pathname).toEqual('/admin/CareTeams/308');
Why was this removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the path, we're testing for the querystring that has been added to the path, hence replaced pathname with search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that, but is the previous assertion no longer necessary, why was it there initially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially, view-details was just creating a path using the care team url plus the id of the care team hence getting the pathname to match was the assertion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mutuajames nice work so far, I however do not think the test changes done show that the reported issue is resolved.
You want the test to show that:
If you call userSearchParams().addParam with a key that already exists on the url, the param value is changed instead of appending to whatever was there initially.
addParam only adds one keyValue pair to url at a time and commits this to the url addParams can add several keyValue pairs in a single call before commiting to url
Checks if weve appended a url before appending another.
closes #1216