Skip to content

Conversation

rzats
Copy link
Contributor

@rzats rzats commented Feb 28, 2024

Summary of changes:

  • prevents tutorial "tour" from automatically reappearing after first visit
  • replaces default signal with a live FluView example instead of a static wave
  • adds "autozoom" functionality to keep entirety of signals within the canvas view
  • inverts colors of "active" toggle buttons for visibility
  • increments "minor" version number (2.0.x-->2.1.x)

Fixes:

@rlunde rlunde requested a review from melange396 March 1, 2024 17:25
Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a preview instance/installation for this branch somewhere?

I think we need make the automatic fitData() calls a toggle-able option, perhaps its own "mode" (joining the three existing modes already available in the top menu). For instance, if a user is looking at a number of signals in a particular time window, and they turn some off and on to get a better look at the other signals, they will lose the specific view they had.

One way to handle this would be that we could switch to "auto-fit mode" when adding a new signal to the canvas (but not when simply toggling an existing signal). While in "auto-fit mode", clicking the mouse inside the view canvas or using the scroll wheel should switch to "pan mode". It might be nice to be able to opt out of "auto-fit mode" when adding new signals, which could be done with a checkbox in the signal load dialog.

@rzats
Copy link
Contributor Author

rzats commented Mar 20, 2024

Notes from the meeting:

  • Reword the comment about include/exclude to make sure it refers specifically to the dataset variable
  • Add a button to disable the new autofitting (but it's enabled by default)

@rzats
Copy link
Contributor Author

rzats commented Mar 27, 2024

@melange396 updated this PR as well!

@rzats rzats requested a review from melange396 March 27, 2024 14:28
Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes:

  • https://cmu-delphi.atlassian.net/browse/OKRS24-62 looks done, the tour doesn't show up every time and now there's a button to show it later
    • However the 'h' hotkey doesn't bring up the help tour (other keyboard commands work for me)
  • When I click import datasets from API and I select FluView, the app hangs indefinitely, which didn't happen before. Something about the way the initial dataset is loaded might be off?
  • https://cmu-delphi.atlassian.net/browse/OKRS24-61 the sample signals is removed, but I'm not sure what "match defaults with COVIDcast dashboard" means; do we want FluView to be the default? if so, this is done, if not, then we should decide what to display
  • https://cmu-delphi.atlassian.net/browse/OKRS24-63 zoom to signals seems to work well, I've tried importing a number of signals and sources, turning them on and off works quite seamlessly
  • I see the EpiVis sample is still in the menu, though deactivated. We should probably remove this completely, seems unnecessary and distracting.
  • I would choose a different icon than a gear for the autofit functionality. Gear made me think it was a "general settings" button. Maybe something more like this or like this?

@rzats
Copy link
Contributor Author

rzats commented May 22, 2024

@dshemetov this is ready for a re-review! A couple of notes:

the 'h' hotkey doesn't bring up the help tour

Fixed!

When I click import datasets from API and I select FluView, the app hangs indefinitely, which didn't happen before. Something about the way the initial dataset is loaded might be off?

This would happen before if you were to try loading the same dataset twice. I added an extra check for that.

I'm not sure what "match defaults with COVIDcast dashboard" means; do we want FluView to be the default? if so, this is done, if not, then we should decide what to display

IMO this is done, but I can set the default to something else as well.

I see the EpiVis sample is still in the menu, though deactivated. We should probably remove this completely, seems unnecessary and distracting.

Done!

I would choose a different icon than a gear for the autofit functionality. Gear made me think it was a "general settings" button. Maybe something more like this or like this?

Done, went for the up-down icon.

@rzats rzats requested a review from dshemetov May 22, 2024 14:26
Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My previous concerns have been addressed, thanks!

If we want to be strict with the first JIRA ticket:

  • "match defaults with COVIDcast dashboard" - the default signal for COVIDcast dashboard is COVID Hospitalizations, so that would be better
    • a subissue here is that I can't seem to select US National when using the COVID Hospitalization dataset; is this a bug or an api limitation or something else?
    • this issue might be moot, since COVID Hospitalizations is out of date and we should use a different signal as default on the dashboard anyway
  • I'm getting very strange behavior with automatic resizing when adding the COVID Hospitalizations as a dataset; when the dropdown for the COVID Hosp signals is closed, the sizing is right, but when the dropdown is open, the sizing gets distorted, the plot stretches down (see the images below). It's like it considers the bottom of the scrollbar on the left to be the bottom of the viewport on the right?
image image

@rzats
Copy link
Contributor Author

rzats commented May 31, 2024

@dshemetov

a subissue here is that I can't seem to select US National when using the COVID Hospitalization dataset; is this a bug or an api limitation or something else?

It is indeed an API limitation! That dataset works on the state level only. It seems to be a subset of the overall HHS dataset, which does offer more geo resolutions.

very strange behavior with automatic resizing when adding the COVID Hospitalizations as a dataset

This seems to be a CSS issue: if too many datasets are added to the left sidebar, it overflows offscreen. I've fixed this with the latest commit!

@rzats rzats requested a review from dshemetov May 31, 2024 12:46
Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, but there are things id like to look at before we merge.

  • I made a number of suggestions for verbiage on the "tour", a few readability suggestions, and one for inverting the color on toggled buttons.

  • I see what youre doing with the include and exclude arguments to Chart.svelte:fitData(), but something tells me those shouldnt be necessary... I thought i figured out some ways around it and put them into #41, but now i cant get that to generate a Netlify preview to test. Do you know what i am missing? Do you think that approach has merit?

  • This current implementation doesnt do the automatic "fit" operation on page load when a signal is specified in the base64-encoded url (such as this one; it requires a post-load press of the "fit data to screen" button before the signal is visible). I think my exploration in #41 might address this (or open a easier avenue to accomplish it), but as i said, its not currently rendering a preview.

  • obviously not a result of the changes in this PR, but worth mentioning: there is a typo which alters the tooltip behavior for one of the buttons, please s/tootlip/tooltip/ on

And then there are some things that dont need to be in this PR, but i will make separate issues for them:

  • support time_type:week (aka EpiWeek format time values) for covidcast signals (first priority is to support them in signals encoded in url param, then to support them in the signal picker) -- see
    time_type: 'day',
    time_values: epiRange(firstDate.covidcast, currentDate),
  • turn autofit into a mode, as mentioned in #36 (review)

@@ -28,7 +30,7 @@
{#if expanded}
{#each node.datasets as child (child.title)}
{#if child instanceof DataSet}
<TreeLeafNode node={child} />
<TreeLeafNode {chart} node={child} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz name attribute/arg for clarity

Suggested change
<TreeLeafNode {chart} node={child} />
<TreeLeafNode chart={chart} node={child} />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Svelte linter actually reverts this change when I try to apply it :) I could disable this in the config, but shorthand attributes like this show up a lot in the code base, e.g. a couple lines before:

<side class="left" {style} data-tour="browser">
                   ^^^^^^^

and elsewhere:

<select class="uk-select" required {name} {id} bind:value>
                                   ^^^^^^ ^^^^

So I think it might be fairly clear what's going on with these.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont love it, but if thats the preferred style in Svelte, we might as well go with it.

rzats and others added 5 commits July 18, 2024 17:20
…nstead of 'datasets' argument (#41)

* revert Chart.svelte

* remove 'datasets' argument from Chart component, replace with reactive reference to 'activeDatasets' store

* revert chart.fitData() calls in TreeLeafNode component

* Force reactive updates

---------

Co-authored-by: Rostyslav Zatserkovnyi <[email protected]>
@rzats
Copy link
Contributor Author

rzats commented Jul 18, 2024

@melange396 applied all of your suggestions except a particular codestyle one!

include and exclude arguments in Chart.svelte:fitData()

These are removed in #41...

no automatic "fit" when a signal is specified in the base64-encoded URL

...and I doublechecked that this is now working as well.

@rzats rzats requested a review from melange396 July 18, 2024 17:08
Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few boolean comparison nits, but otherwise this looks fantastic and i think its ready to go!

@@ -28,7 +30,7 @@
{#if expanded}
{#each node.datasets as child (child.title)}
{#if child instanceof DataSet}
<TreeLeafNode node={child} />
<TreeLeafNode {chart} node={child} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont love it, but if thats the preferred style in Svelte, we might as well go with it.

@melange396
Copy link
Contributor

You should probably also bump the version number for this:

"version": "2.0.3",

@rzats rzats requested a review from melange396 July 30, 2024 13:15
@rzats
Copy link
Contributor Author

rzats commented Jul 30, 2024

@melange396 applied the changes!

Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@melange396 melange396 merged commit 422769a into dev Jul 30, 2024
6 checks passed
@melange396 melange396 deleted the rzatserkovnyi/epivis-upgrade branch July 30, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants