Skip to content
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

Implement date search that works intuitively with new date range fields #3285

Closed
corneliusroemer opened this issue Nov 25, 2024 · 15 comments · Fixed by #3308
Closed

Implement date search that works intuitively with new date range fields #3285

corneliusroemer opened this issue Nov 25, 2024 · 15 comments · Fixed by #3308
Labels
feature Feature proposal website Tasks related to the web application

Comments

@corneliusroemer
Copy link
Contributor

We will soon no longer have a single date field: there will be two dates for each sample instead: lower range and upper range of what the date string we got means. For complete dates, the 2 are the same. But for incomplete they aren't.

This makes search non-intuitive to users. Most users will initially not care about the concept of date ranges, they just want something that works for filtering dates somehow.

What we get instead after #3263 is two dates to filter by, and this causes cognitive strain (even to me having thought about this feature for weeks).

image

There's a simple way of having intuitive search: show just one date search bar with two modes (selectable through radio button):

  • strict (sample date range must be entirely contained inside the query range)
  • lax (sample date range must at least overlap the query range)

People probably want strict to be the default, but we can change this once we know what's more intuitive. The idea to have strict and lax search is something that I haven't seen much on other websites so even this way will take some getting used to (the good thing is that people won't need to know about strict vs lax, it'll just intuitively work ok).

Implementation:

  1. Create a new custom search field type date_range (exact names tbd, just for scoping I choose some)
  2. It takes as config two config values: date_lower and upper, to refer to the metadata fields that should be used
  3. It displays a normal date search but with added radio buttons for "strict" and "lax" modes, defaults to strict. For what follows I call the fields range_min and range_max
  4. The LAPIS query is constructed based on mode as follows in pseudocde
if strict:
    filter((date_lower >= range_min) and (date_upper <= range_max))
if lax:
    filter((date_lower <= range_min) and (date_upper >= range_max))

That's it, I hope that's clear and should be doable. Let me know if you have questions.

@corneliusroemer corneliusroemer added website Tasks related to the web application feature Feature proposal labels Nov 25, 2024
@corneliusroemer corneliusroemer added this to the Priority 1 (highest) milestone Nov 25, 2024
@corneliusroemer corneliusroemer moved this to Prioritized in Planning Nov 25, 2024
@theosanderson
Copy link
Member

The strict/lax setting will need to be persisted in the URL like other aspects of search parameters (but will be the first such thing that is per-field but is not a search parameter passed to LAPIS).

@corneliusroemer
Copy link
Contributor Author

@theosanderson can you explain why it needs to be persisted? For UI/UX reasons? It's just a radio button? What am I missing?

@theosanderson
Copy link
Member

The page is set up such that you can send the link of your current page to someone and they will see what you see (or refresh the page, or bookmark, etc.)

@corneliusroemer
Copy link
Contributor Author

Right, and that includes the exact state of the search facet widgets.

To keep that up we'd need to add a field for radio state and it wouldn't be entirely unambiguous to deduce which date field has the entry. Is that a strict requirement of the code or something we could drop in that case?

@theosanderson
Copy link
Member

To keep that up we'd need to add a field for radio state

Whether this is a "field" in the conventional sense I don't think is determined? It could be a special thing, like page currently.

it wouldn't be entirely unambiguous to deduce which date field has the entry

It would be unambiguous if we named the query param specifically per field?

Is that a strict requirement of the code or something we could drop in that case?

We've put a fair amount of effort into this behaviour, so it would feel strange to me to give up on it now

@corneliusroemer
Copy link
Contributor Author

Whether this is a "field" in the conventional sense I don't think is determined? It could be a special thing, like page currently.

Sorry for being imprecise, yes that's what I meant, a URL param of some sort

It would be unambiguous if we named the query param specifically per field?

I don't think so unless we decide to only surface date lower and upper search as an integrated component and not as optionally a joint one and optionally individual one. It might make sense to not allow anything but integrated, however, for simplicity.

There's not that much use for combinations that are neither strict nor lax, in theory one can specify 4 combinations of (cartesian of from/to x lower/upper), but strict lax are the only 2 really sensible ones.

We've put a fair amount of effort into this behaviour, so it would feel strange to me to give up on it now

It would only be unambiguous if we allowed both the joint upper/lower strict/lax search type to be selecteable as well as upper/lower individually as classic date searches.

If we were to only support joint mode we wouldn't actually need that new URL param, the radio button state would be encoded into the combination of: upper/lower x from/to present in the search:

If one of lowerFrom=&upperTo= is present it's strict, one of lowerTo=&upperFrom= it's lax. If none present it's default and all empty.

How does that sound?

@theosanderson
Copy link
Member

It might make sense to not allow anything but integrated, however, for simplicity.

I was assuming date search would continue to be a single field - I think anything else would be quite confusing.

If we were to only support joint mode we wouldn't actually need that new URL param, the radio button state would be encoded into the combination of: upper/lower x from/to

Yes I haven't worked that through myself, but it makes sense on the face of it - sounds OK!

@corneliusroemer
Copy link
Contributor Author

I was assuming date search would continue to be a single field - I think anything else would be quite confusing.

Well it ceases to be single field as soon as we hit merge on the ranges :)

Yeah I think I'm quite happy with the solution above, it should work. I wasn't aware of the complication of URL <-> state mapping but I think we've resolved it pretty much.

@fhennig
Copy link
Contributor

fhennig commented Nov 26, 2024

That's it, I hope that's clear and should be doable. Let me know if you have questions.

Yep, I get it! I just think I would have defined the lax/partial overlap differently:

I am just not sure about your definition of lax. There are these four overlap situations:

A
sample range:       |--|
search range:      |-----|

B
sample range:  |-----|
search range:      |-----|

C
sample range:      |-----|
search range:   |-----|

D
sample range:  |---------|
search range:    |-----|

Intuitively I'd say: strict mean only A is accepted: The sample collection date must be in the range I search for. And for lax I'd say: B, C and D (EDIT: and of course A) are accepted: The sample might have been collected in the range I search for.

You defined lax as:

(date_lower <= range_min) and (date_upper >= range_max)

but that would only include D (not even A)

Now that I wrote all this, I think that might just have been a mistake on your end. Instead I guess we'd define lax as this:

  (date_lower <= range_max) and (date_upper >= range_min)

If we'd go with this definition of 'lax', I'm not sure what I'd name the parameters.

I hope I understood what you meant!

Oh, but the general idea of it seem pretty clear to me 👍

@emmahodcroft
Copy link
Member

not 100% sure I'm following but my thoughts, for whatever they're worth:

  • While I think strict/lax is cool, I am not sure I agree it's like super super urgent after merge of the date ranges, but it would be nice to not wait too long to have it (yes it's not 100% intuitive but I also think the lax/strict isn't necessarily 100% intuitive either - basically, I think date range are complicated however you slice them)
  • I do think we should stick to reflecting everything in URL, it would be weird not to include this and particularly unintuitive since we do it with everything else
  • Felix's definition of lax & strict make sense to me (but lax would also include A in your main example, right?)

@fhennig
Copy link
Contributor

fhennig commented Nov 26, 2024

but lax would also include A in your main example, right?

Oh yes! I'll edit that in

@theosanderson
Copy link
Member

@emmahodcroft if we don't have strict/lax (which I agree is an option) we still need changes to search code, and to decide which of strict or lax to use (do you have a preference?)

@corneliusroemer
Copy link
Contributor Author

Thanks @fhennig for catching my logic error, indeed the following is correct as you state:

(date_lower <= range_max) and (date_upper >= range_min) 

Felix' solution of using a checkbox instead of radio works quite nicely as well, it makes lax the default (unnamed) and offers strict as an option via checkbox. The only confusion now is that the tooltip introduces "lax" as a concept but this isn't visible in the UI at all as it's the implicit default. I don't think I'm too worried about this, just pointing it out.

@theosanderson
Copy link
Member

theosanderson commented Dec 2, 2024

The only confusion now is that the tooltip introduces "lax" as a concept but this isn't visible in the UI at all as it's the implicit default. I don't think I'm too worried about this, just pointing it out.

Actually the default seems to be strict in Felix's PR, which makes sense to me - if you reduce the date range to 1 week, you don't (by default) want sequences with just the year specified appearing. And "not strict" is explained in a tooltip, which seems reasonable to me.

@corneliusroemer
Copy link
Contributor Author

Right, my memory was incorrect. Strict is checked by default and the unchecked version is called not strict as opposed to lax. Fine with me!

@github-project-automation github-project-automation bot moved this from Prioritized to Done in Planning Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature proposal website Tasks related to the web application
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants