nextstrain / flu_frequencies

Flu clade and mutation frequencies
https://flu-frequencies.vercel.app
3 stars 3 forks source link

Plot tweaks #11

Closed ArtPoon closed 1 year ago

ArtPoon commented 1 year ago
vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
flu-frequencies ✅ Ready (Inspect) Visit Preview May 15, 2023 11:51am
ArtPoon commented 1 year ago

Thanks @ivan-aksamentov - I've since found some bugs that arise when the frequency estimate is 1 (causes a division by zero error because I'm relying on this quantity to scale to the plot region)

ivan-aksamentov commented 1 year ago

@ArtPoon By Richard's request I just merged branch web to master. So the PRs would now need to go to master.

ArtPoon commented 1 year ago

Do you guys use a primary dev branch or can a PR come from any branch?

ivan-aksamentov commented 1 year ago

@ArtPoon master branch is the dev branch. For web apps we typically do the following 3-branch strategy:

(examples are with Nextclade because flu_frequencies does not have deployment environments or even domain name yet)

ivan-aksamentov commented 1 year ago

I made a few refactorings to address linter warnings and improve type-safety (hopefully). Technically this looks good to me.

However, now that I learned how it works, I find the UX a bit strange. The bubbles introduce quite a bit of visual load, and they are displayed unconditionally. Then the bubbles are filled on mouse hover when the ranges are not shown, but not filled when ranges are shown. Similarly, the lines are shown in one mode and not another.

I understand that hollow bubbles allow to see through them better. But this all seems quite random to me. As a user I would not be able to tell what to expect every time I hover a mouse and why it is done this way.

I propose to consider (perhaps in followup PRs) to add checkboxes or switch toggles to enable and disable the bubbles and confidence lines independently, without attaching this to one another and to confidence ranges (they will also stay independent). Some users have very large displays and they browse in full screen mode, so they can have preferences different from someone browsing from a phone. Ability to configure plot components independently would offload UX decisions away from us to the end users.

Additionally, in this application we have 2 types of plots: for regions and for variants. Which are really the same plot, but with different data. For symmetry, is the bubbles and lines something that also makes sense in the other type of plots?

In any case, I will let Richard and Cornelius to comment on the scientific side and UX of this feature as well as its further extensions, and to make the final decision.

ArtPoon commented 1 year ago

Okay fair point about giving users the option to turn off the bubbles. I will have line segments displayed whether or not shaded regions are being displayed instead of switching to filled cirlces, i.e., previous behaviour. I was going to port these interface changes to the variant-specific plot after drafting the implementation in the region-specific plots.

ivan-aksamentov commented 1 year ago

@ArtPoon In https://github.com/neherlab/flu_frequencies/pull/16 I also scaffolded the checkboxes and the state required for it, in case you want to go this way. Feel free to take over and use all of it, or a part of it or none of it.

ArtPoon commented 1 year ago

@ArtPoon In #16 I also scaffolded the checkboxes and the state required for it, in case you want to go this way. Feel free to take over and use all of it, or a part of it or none of it.

Well shoot, I just implemented checkboxes already and didn't see your message :-(

ivan-aksamentov commented 1 year ago

@ArtPoon Haha! No worries!

ArtPoon commented 1 year ago

I'm trying to resolve two next-dev.js errors that are thrown when loading a region or variant page, respectively. For region page, we get the following:

Warning: Can't perform a React state update on a component that hasn't mounted yet. This indicates that you have a side-effect in your render function that asynchronously later calls tries to update the component. Move this work to useEffect instead.

This seems to be associated with the following line: https://github.com/neherlab/flu_frequencies/blob/877f244c97a76151a5fe03ceecea5f3e08761c0e/web/src/state/variants.state.tsx#L20-L29C3

ArtPoon commented 1 year ago

Hm, never mind - I'm getting the same errors on master branch, so it doesn't seem to be my fault :-)

ArtPoon commented 1 year ago

Thanks @rneher! @ivan-aksamentov is this ok to merge?