-
-
Notifications
You must be signed in to change notification settings - Fork 61
Add archived item tag filter #2855
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
Conversation
... while also preserving existing behaviour for `state` parameters specifically, so that this isn't a breaking change
archived items are accurate this counts against all archived items for the org that might be displayed in the archived item list (i.e. those with successful states)
tw4l
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.
Reviewed backend and tested locally, all looks good! Would be nice to add test coverage for the new endpoints and filters.
…m/webrecorder/browsertrix into frontend-archived-item-tag-filter
SuaYoo
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.
I like the new controller! My only feedback is that as a controller API user it's not immediately clear to me that setting this.query.value would result in a component update, since .value doesn't appear like an object reference.
What do you think about adding a public setValue instead? Then this.query.setValue(e.target.value) feels more like a predictable update, potentially reducing the cognitive load of remembering that this.query.value will trigger a re-render for a component that may have many other object properties/state objects to keep track of.
I could see that working! I was hoping for an API where you could just directly get/set the class instance itself, which I think could probably be achieved with a proxy? Maybe I'll give that a try too, idk, I want it to feel like a plain property ideally. |
|
Actually now that I think about it, maybe it's possible with a decorator instead? That would probably be a little nicer, I'll give that a go. |
ikreymer
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.
Looks great!
|
Didn't really get anywhere with the decorator idea — I think it's possible, but it requires digging deeper into lit internals than I think I want to do right now. Happy to revisit in the future though! For now I'm happy to go with the |
Closes #2853
Changes
Adds a tag filter to the archived items list.
Along the way, this PR...
/orgs/{oid}/all-crawls/tagCountsendpoint for displaying tag counts of archived items, much like the/orgs/{oid}/crawlconfigs/tagCountsendpoint that exists for tags on workflowsSearchParamsValuecontroller that automatically converts values to and from search params using an encoder/decoder pair. This simplifies a bunch of the complex logic implemented in the workflow list controls — I intend to rewrite those using this controller in the near future as well.SearchParamsControllerI wrote a little while ago, it automatically pushes history to the stack, and so the browser's back/forward buttons work as expected for any value hooked up to this!Screenshots
Testing
As this PR contains both frontend and backend changes, both parts need to be deployed together to test this PR.
To test:
Next Steps
SearchParamsValue, and the search input should be hooked up to it as well