-
Notifications
You must be signed in to change notification settings - Fork 19
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
Feat: Use url params to store state in Checks list page #797
Conversation
75befd6
to
193750d
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.
I am super excited that this works and you got it up so quickly!
Just some feedback to address to make this as durable as we can and make it something that endures and is re-usable in the long-term.
If you want some reference material, this is how it's been added in the k6-app. By no means have to use it, just something to consider 😄
expect(historyPushMock).toHaveBeenCalledTimes(2); | ||
expect(historyPushMock).toHaveBeenLastCalledWith({ search: `anotherKey=${encodeURIComponent('"anotherValue"')}` }); | ||
}); | ||
}); |
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'm interested in if these unit tests endure after addressing the feedback. If they all break I'm not that bothered if you don't want to replace them. 😅
I love tests but I am less fussed about unit tests that are testing implementation details and more interested in testing user journeys and user-facing problems rather than developer centric ones. Some unit tests are great and totally necessary in FE but as we have the user-facing tests you've added for the checks list page (which will mostly endure through implementation detail changes) these matter less.
We can talk about this more later if you want 😄
- support to either push/replace on history - remove usage of useEffect for setting a parameter which fixes back/forth buttons - still pending to implement a better parsing strategy instead of using JSON.parse
There's still pending work on the "filters" param:
This is to prevent having a huge object with the "filters" key Now we have a key for each filter separately, like labels=,type=,status=, etc.
- Probes encode/decode functions still need a refactor - The idea is it doesn't rely on JSON.stringify/parse
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.
Great work! Pure 🔥🔥🔥🔥🔥🔥🔥
I'm approving even though I've left two minor comments, feel free to merge after you've either addressed or dismissed them= 😄
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.
🔥🔥🔥🔥🔥🔥
This PR implements applying list filters (such as search term, status, labels, probes), sorting selection and view type using the url query parameters instead of local storage. This brings a number of benefits such as being able to share the page maintaining the filters selection, being able to use the back/forth buttons, and also preventing unwanted behavior in certain scenarios.
Fixes #762