-
Notifications
You must be signed in to change notification settings - Fork 46
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(SystemsPage): RHINENG-5977 - Fix URL params filters handling #2129
base: master
Are you sure you want to change the base?
Conversation
Referenced Jiras: |
8edcd8e
to
6811d96
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2129 +/- ##
=======================================
Coverage 67.15% 67.16%
=======================================
Files 128 129 +1
Lines 3440 3441 +1
Branches 1067 1068 +1
=======================================
+ Hits 2310 2311 +1
Misses 1130 1130 ☔ View full report in Codecov by Sentry. |
6811d96
to
20314d9
Compare
20314d9
to
d65a741
Compare
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.
Codewise looks good. However, while testing I found out that if I apply some filters and open another tab with the same URL, those applied filters are lost. This needs to be fixed
import { useCallback } from 'react'; | ||
import { useSearchParams } from 'react-router-dom'; | ||
|
||
// TODO DRY:similar to constructParameters |
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.
Maybe it is time to finally to implement this TODO
. Can we try to consolidate this function with constructParameters
and get rid of the duplicate code?
const [searchParams, setSearchParams] = useSearchParams(); | ||
|
||
const setUrlParams = useCallback((parameters) => { | ||
const para = constructURLParameters(parameters, allowedParams); |
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.
NITPICK: It looks like a typo on the first look.
const para = constructURLParameters(parameters, allowedParams); | |
const param = constructURLParameters(parameters, allowedParams); |
describe('MiscHelper', () => { | ||
it('useUrlParams', async () => { | ||
const { result } = renderHook(() => useUrlParams(['a', 'b']), { | ||
wrapper: createTestWrapper(MemoryRouter, { initialEntries: ['/'] }) |
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.
Can we use the TestWrapper which has already MemortRouter provided?
@@ -22,4 +22,10 @@ TestWrapper.propTypes = { | |||
store: propTypes.object | |||
}; | |||
|
|||
export const createTestWrapper = (Wrapper = TestWrapper, props) => { |
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.
If we use the shared wrapper, this is not needed.
@bastilian Just a friendly ping on this :) |
This fixes the issue with filter params and adds them correctly to the URL.
How to test: