search

cmu-delphi / www-covidcast

Front end for interactive visualizations powering the COVIDcast website.
https://delphi.cmu.edu/covidcast/
MIT License
13 stars 2 forks source link

Set COVIDcast export values based on URL parameters #1244

Closed rzats closed 4 months ago

rzats commented 6 months ago

Prerequisites:

Summary

Fills in the form values on the COVIDcast Export Data page based on URL parameters. This will be used to integrate the export UI with e.g. the EpiVis dashboard or signal documentation, with certain values being pre-filled based on parameters selected on another section of the website.

To test this out, visit the export dashboard on the preview website, then append this set of parameters to the URL:

/?data_source=covid-act-now&signal=covid-act-now-pcr_specimen_total_tests&start_day=2024-01-01&end_day=2024-02-02&geo_type=state
netlify[bot] commented 6 months ago

Preview link ready!

Name Link
Latest commit 333a79c85f0a971045d1ba747b47f818626d1372
Latest deploy log https://app.netlify.com/sites/cmu-delphi-covidcast/deploys/66043c392577910008596279
Deploy Preview https://deploy-preview-1244--cmu-delphi-covidcast.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

melange396 commented 5 months ago

clarification: the dates in the url and in the page seem to be off by one for me --

image
rzats commented 5 months ago

Notes from the meeting:

rzats commented 5 months ago

@melange396 fixed up all your suggestions (except the API key to allow access to restricted signals - I think that's best done in a separate PR, since it's not really related to URL params)

Here's a new URL to test out the fix to the issue you've mentioned: https://deploy-preview-1244--cmu-delphi-covidcast.netlify.app/export/?data_source=covid-act-now&signal=covid-act-now-pcr_specimen_total_tests&start_day=2024-01-01&end_day=2024-02-02&geo_type=state

In short - this was a bug with inactive sensors in particular, where the datetime selector was incorrectly set to current (e.g. 2024) dates for inactive sensors (so e.g. only running between 2020-2021). It was possible to reset the dates to valid ones by re-selecting the date on the picker, but a bit unintuitive.

I fixed this by setting the datetime selector to the sensor's minimum and maximum start time, if it's inactive.

melange396 commented 5 months ago

Start date field is still showing off-by-one problem, end date field looks like its bound to the metadata (even though specified date is inside the valid range): image

melange396 commented 4 months ago

Why was this merged? There are still unaddressed concerns, plus there is no reason to release bugged and undertested functionality.

dshemetov commented 4 months ago

I thought Rostyslav addressed most of the concerns from your previous review? My personal preference here is to not let the perfect be the enemy of the good. Rostyslav made an issue on Jira to track TODOs on this feature.

dshemetov commented 4 months ago

But as a broader point, I'm not clear about our approval process for these PRs. In general, Rostyslav, we should probably wait for George's input, even if I give a more recent review. I'm writing my reviews from my personal perspective on the issue, but I'd like to give George the opportunity to give his take. I'm a bit out of the loop on things on the front-end / dev side, always happy to give input and take some burden off of George, but I do think that George has a better view on what needs to get done and when things are ready to merge.

rzats commented 4 months ago

Things to fix up:

dshemetov commented 3 months ago

Also this one: