-
Notifications
You must be signed in to change notification settings - Fork 45
Filter params in url #469
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
base: master
Are you sure you want to change the base?
Filter params in url #469
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #469 +/- ##
==========================================
+ Coverage 93.42% 94.04% +0.62%
==========================================
Files 5 5
Lines 365 403 +38
==========================================
+ Hits 341 379 +38
Misses 24 24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dantownsend
left a comment
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.
Thanks for this. It's cool how it syncs the filter values to the URL params.
But I think the missing piece is the reverse - so if you copy the URL, and post it into a new tab, it should then take the URL params, and populate the filter sidebar.
But it doesn't work with or without PR changes (with current code in main branch) even in the same tab. As soon as we close the filter sidebar (and open again), the form fields are empty. |
|
@sinisaos That's cool, thanks! I've tested it locally, and it works great, besides the select being populated, as you mentioned. I haven't been able to figure that out yet. |
|
@abhishek-compro That makes sense, but it doesn't really affect anything. If I remove that line entirely, the result is the same because
I've experimented with |
|
@dantownsend Now the filter_query_params.mp4 |
…nstance is created, before the DOM is rendered
|
@sinisaos This looks great, thanks! I've done some testing, and it seems to work for all field types now? The only thing I'd like to add in a future PR is also syncing the page number to the query params too. |
Yes. It should work for all field types.
Yes, that would be nice. I can try to do that, but you need to merge this PR and the pagination PR first, because there's no point in trying to make those changes now, if the pagination is going to change. |
Related to #466