Bucket list filter changes#207
Conversation
…uttons and filter by triage status, change url hash from query string to a different format, separate Advance query mode
jgraham
left a comment
jgraham
left a comment
There was a problem hiding this comment.
I think the only blocking comment here is about using Logged as a label. But I'm not sure that this frontend design is going to scale well to more kinds of search, and I also think the code is likely to need a refactor to make other searches work well. Given we know we have requirements around country-specific searches and so on I think we should consider doing that work sooner rather than later.
| @@ -0,0 +1,41 @@ | |||
| export const BUCKET_STATES = [ | |||
There was a problem hiding this comment.
Why is this a list and not a mapping like {key: {label, queryFields}}?
| 0: query, | ||
| 1: { | ||
| op: "AND", | ||
| 0: { op: "OR", domain, domain__endswith: "." + domain }, |
There was a problem hiding this comment.
It's a bit unclear to me why this works when there isn't a domain filter; it seems like in that case it's either going to match the empty string or a domain that ends with ".". Clearly it does work however :)
I think for the purposes of future extension it would be good to create a query builder object that creates a query based on the fields that are part of the query.
|
|
||
| export const parseHash = (hash) => { | ||
| return hash | ||
| const result = {}; |
There was a problem hiding this comment.
I don't love the way that the handling of each part of the filter is now split across multiple files, so adding new filters requires you to update code in lots of places. In particular buildHash requires you to know specifics about what filter properties are supported.
| const parts = []; | ||
| if (activeState !== "needs_triage") parts.push(`status=${activeState}`); | ||
| if (domainFilter) parts.push(`domain=${encodeURIComponent(domainFilter)}`); | ||
| if (triageStatus) |
There was a problem hiding this comment.
I think we should start linting against this if-without-braces style.
| ); | ||
| .filter(Boolean) | ||
| .forEach((part) => { | ||
| const eqIdx = part.indexOf("="); |
There was a problem hiding this comment.
What if it doesn't contain an =? I guess key ends up being the empty string and value ends up being whatever's in that part of the query which is sort of fine, but it's not obvious from the code if it's intended or not.
| queryFields: () => ({ bug__isnull: true, triage_status__isnull: true }), | ||
| }, | ||
| { | ||
| key: "logged", |
There was a problem hiding this comment.
I think "Logged" is a bit unclear. AIUI the semantics are "has a bug".
| <template> | ||
| <div> | ||
| <div class="btn-group" role="group" aria-label="Bucket status filter"> | ||
| <button |
There was a problem hiding this comment.
When I look at this locally it doesn't quite make sense to me. "Needs Triage" and "Triaged" are obviously different, but "Logged" seems most like a subset of "Triaged" (presumably untriaged buckets don't have bugs").
|
|
||
| <div class="filter-row"> | ||
| <div class="input-group input-group-sm domain-input"> | ||
| <span class="input-group-addon"><i class="bi bi-globe2"></i></span> |
There was a problem hiding this comment.
Having two icons on this control makes it feel busy. I'm also not a huge fan of the only clue about what the control does being placeholder text. If we add a second text input, having the search button on this one won't work.
If we want to make things visually simple we could go down the github route where you write the search as domain:youtube.com label:wordcup status:untriaged or something like that.
2 participants
I've made these changes:
#status=triaged&domain=example.com&triage_status=worksforme. It also supports multiple items with the same key, for future support for labels i.e .#status=triaged&label=nsfw&label=-worldcup2026.