-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add options to edit min/max dates #1412
Conversation
@jameshadfield here is an implementation of @huddlej's suggestion on using |
Thanks @victorlin - I will try to review this tomorrow 🎉 Testing is a bit hit-and-miss at the moment (the integration tests are mostly broken); we do have some unit tests but briefly looking at the code here I don't see any obvious candidates. (Testing is something that'd be nice to improve going forward!) |
@jameshadfield Could testing simply be done on the end product? I'd be curious to check out what @victorlin came up with but couldn't immediately find a CI deployment. Did I miss something? |
@corneliusroemer looks like now there is a deployment here: https://auspice-add-date-picker-bhcm7u.herokuapp.com/ |
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.
Thank you for contributing this, @victorlin! I tested the date pickers in a local Auspice installation and they generally worked as expected. I think the functionality in this PR addresses the original issue sufficiently to merge now (other code style/implementation reviews from @jameshadfield notwithstanding).
A couple of minor thoughts occurred to me while using the pickers (not anything I'd recommend addressing in this PR):
- I wish I could click on the text of the current min/max date itself to activate the date field edit mode. I found myself repeatedly clicking or doubleclicking on the text before remembering I had to click the little calendar icon.
- I was able to enter invalid dates (like a date later than the max date) by typing it into the date field. The date picker's calendar correctly prevented me from picking a later date though.
Thanks for the suggestions @huddlej!
Good point – I originally had text clickable before adding the icon. It's a simple change to include the text as well. I could add it here, or create a new PR following merge so we can compare both interfaces.
This is also achievable via passing an invalid date in the URL (e.g. https://nextstrain.org/ncov/open/global?dmax=2050-01-01) though it looks like just a labeling issue – the visualizations still render within absolute bounds. It could be solved by post-processing on the |
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 looks great! I have some minor syling requests, and also echo @huddlej's desire for the input to be edible when clicking on the date text itself (with cursor: pointer
on the text also).
Re: invalid dates. As you say, this is possible via the URL so it's not a blocker for this PR, however we should fix this in later work.
Let's squash and merge this once approved, since the commit history is a bit verbose. |
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.
Approved (finally 😬 ) pending removal of the merge commit and (optional) squashing some commits together
I just realized this doesn't work with negative dates. The HTML date picker (at least on a couple browsers I've tested) won't let you pick earlier than Would this be appropriate to document as a feature gap somewhere, or should we reconsider implementation? |
Choosing precise BCE dates is a bit ridiculous, in my opinion, so I don't think it's a problem. Perhaps we could hide the datepicker when the date is BCE? |
Good idea, I'll make a new PR for that. |
I bet this will make @BryanTegomoh's day when it gets officially released! Thank you for pushing this through, @victorlin and @jameshadfield! |
Description of proposed changes
Currently, the date range is only customizable by either interfacing with the slider (has limitations) or editing the URL directly (hacky, forces page reload).
This pull request adds options to change min/max dates individually.
Usage:
Enter
.Safari:
Brave (Chromium):
Notes:
min
/max
properties.theme
– customizability is limited and the style varies by browser.Related issue(s)
Fixes #1406
Testing