Brush zooming - Githubissues

richardgoater commented 2 years ago

"I made a promise, Mister Frodo, a promise..."

Here is a basic example of brush zooming, only on the country chart atm. There are loads of warnings which I can try to clean if it doesn't build.

I think the x axis ticks need adjustment when the chart is zoomed, and let me know your thoughts on bringing back animation - seems like the animations skip a lot anyway.

I would plan to use a hook plus some shared components to roll it out to the other charts, would say it doesn't look like this can be done in an extensible way unfortunately.

I'm wondering also if you'd prefer the zoom to be "global", I've never used Recoil so I might not be able to do it.

Let me know what you think!

vercel[bot] commented 2 years ago

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

Name	Status	Preview	Updated (UTC)
covariants	✅ Ready (Inspect)	Visit Preview	Jun 30, 2023 7:18pm

emmahodcroft commented 2 years ago

Hi @richardgoater - this is awesome - thanks so much for working on this!!

I have just tried it out, and I think it's really cool. I love how easy it is to zoom and how easy to zoom out. For me it works smoothly and without issue.

I do think the x-axis likely needs some work - I don't see any additional ticks or lines appearing when you zoom, so if you go in far enough you end up in no-mans-land 😆 : So we probably need some way to dynamically come up with some X-axis scaling down to week level (you can't zoom any further than the points available, as far as I can tell, so luckily we shouldn't have to worry about anything finer-grained than this!).

Also, as you can see in the above screen shot, the Jul 2022 seems to stick around no matter where you've zoomed (the time period above is Aug 2021 entirely).

let me know your thoughts on bringing back animation - seems like the animations skip a lot anyway.

I'm not 100% sure what the 'animation' is - is this that each graph 'plots' as it appears? I don't have strong feelings on this - I'm not sure it adds much and would be happy without it, especially if it takes overhead or more complexity. I'm currently not seeing any animation on zooming or zooming out (which is also fine with me - but just noting in case there should be).

I'm wondering also if you'd prefer the zoom to be "global", I've never used Recoil so I might not be able to do it.

My guess is you mean, that you zoom on one chart & all chats zoom? In a wish-world, this would be nice - it would mean we could in theory add URL params (which are always handy) and allow people to make easy comparisons within charts. But, if this is really hard, I also think we can live without it. What you've implemented is already a huge improvement on what we have, so I don't want perfect to be the enemy of good! It could also be something to tackle separately as an 'upgrade' on this one, if it seems like it might be possible but would take much more playing/exploring etc.

I would plan to use a hook plus some shared components to roll it out to the other charts, would say it doesn't look like this can be done in an extensible way unfortunately.

@ivan-aksamentov Will have to decipher this I'm afraid! - but maybe he could also advise on whether there might be a way to do that?

Once again, this is so cool - thank you so much for persisting on this!! 😁

richardgoater commented 2 years ago

Thanks so much for your great comments @emmahodcroft !

Yes the ticks definitely need adjusting, I'll try to iterate on it soon. I reckon the weird stuck tick is due to a hack I implemented last year? It will need to be skipped for this case now.

The charts are supposed to animate between the zoomed in and zoomed out views as well but I think it's only working in development for some reason - I will check this again. It helps to communicate the change visually so I think it's worth having.

yes you're right, one zoom to rule them all 😅. I reckon you already have it working in other parts of the site so I can probably crib from there, agree that it would be great to use the URL.

I always welcome any guidance from @ivan-aksamentov 🙌🏽

ivan-aksamentov commented 2 years ago

@richardgoater

Nice, thank you Richard!

I think the x axis ticks need adjustment when the chart is zoomed

Scaling dates is probably the trickiest part of the feature. Dates are messy, sometimes absent and probably have many corner cases.

let me know your thoughts on bringing back animation

No particular opinion here. Some people may consider the time spent on animation wasteful. But I think I disabled it because there were some visual glitches. Don't remember why exactly.

I would plan to use a hook plus some shared components to roll it out to the other charts, would say it doesn't look like this can be done in an extensible way unfortunately.

Haven't looked at the code very closely yet, but maybe either of this will work:

return the 3 mouse event handler functions from the hook, as well as the state and components necessary for zoom
alternatively, return a ref from the hook, which is a target for mouse events attached inside the hook (using DOM functions, as opposed to passing them into component)?

If nothing works, then keeping a little bit of duplication in 3 places is fine, I think. Perhaps we'll find a way to refactor it later.

I'm wondering also if you'd prefer the zoom to be "global", I've never used Recoil so I might not be able to do it.

Date ranges might be different across different plots, I think, which we may treat as a corner case, or "fix" them to be the same range. But again, date ranges are messy.

Recoil is easy:

consider having a const [foo, setFoo] = useState(fooDefaultValue) hook
add a new atom in the state/ directory, let's say fooAtom: give it a unique name (key) and set the default value to fooDefaultValue (see existing atoms in the directory for examples. This one or this one in Nextclade are simple enough. Ignore atom families and selectors for now).
replace useState(defaultValue) with useRecoiState(fooAtom)
that's it! now the value can be shared across components, persists across component mounts and can be easily serialized into URL params (as it is done with countries and variants, using Recoil effects, e.g. here)

ivan-aksamentov commented 2 years ago

In my experience, using mouse events attached to a component and then storing the mouse state in the components' state is often awkward. Here in particular when you mouse-down on a plot, then move the mouse beyond the plot boundaries, the component enters invalid state. It cannot register mouse-up event anymore and you need to go back to the plot and click down-up for it to unstuck.

There aren't great workarounds I know of. Perhaps we can register a global mouse up event, such that it fires even if mouse is outside of the component's rectangle? There should be some libraries for that. Games often need this.

ivan-aksamentov commented 2 years ago

I also noticed that dragging mouse with mouse down causes text selection to trigger. Gladly it is easily solved with CSS (disabling the selection).

This however might not allow to select the country and variant names at the card headers.

We'd need to double-check that disabling mouse selection does not break full-text search in the browsers (Ctrl+F), because people seems to be using it often to find their country plots on the page.

richardgoater commented 2 years ago

Thanks so much @ivan-aksamentov !

I was also thinking of returning the event handlers etc. from a hook, I will try that first. It will need to be imported into each chart but I don't think we can avoid some change on each one.

Thanks for the great tutorial and examples for Recoil! I reckon globalising the zoom should be very doable now, excited to try it out!

I previously used a guard/hack to make out-of-bounds behaviour slightly better: https://github.com/covince/covince/blob/master/src/components/MultiLinePlot.jsx#L406-L408. I can bring it across and see how it goes.

Think it should be possible to prevent selection on the SVG specifically, but appreciate you pointing out the use-case that the chart headers should remain searchable 👍🏽

Hope to have some progress soon but please feel free to explore some of the issues if you like 🙌🏽

emmahodcroft commented 1 year ago

@richardgoater I know you're still working on this but I just wanted to say that I saw you worked on this recently and I appreciate you coming back to it when you can! 😁

richardgoater commented 1 year ago

Thanks @emmahodcroft ! I was using it to try to pitch Vercel at work so I have been thinking about it, It's a bit tricky for me to work on atm but would love to get this PR done. I think the date controls should be moved to a sticky toolbar like the variant page, and the idea is the zooming on the chart would just be a proxy for using the controls. I couldn't get a proper list of the weeks to use for the slider though, is there are a good way for me to do that?

emmahodcroft commented 1 year ago

No worries at all! Definitely, anytime you can dedicate is awesome, but there's no pressure from our side :) I just didn't want you think your continued efforts went unnoticed!

For the dates, I am not sure we have a guaranteed complete full list, but the min and max dates are in : https://raw.githubusercontent.com/hodcroftlab/covariants/master/web/data/perCountryData.json Should be under regions distributions max_date min_date for region: World entry - I think. They should be divided into 2 week intervals within that, if everything's gone to plan 🤞

richardgoater commented 1 year ago

Ahh awesome, thank you! I'll try to use those when I manage to get back to hacking 🙂

emmahodcroft commented 1 year ago

🎉 🎉

richardgoater commented 1 year ago

hehe thanks @emmahodcroft, still quite a bit to do I think. Unfortunately I'm finding that my Chromebook is struggling to run the dev server, and it's started getting into an infinite loop of warnings before crashing my terminal! Hopefully I can fix this...

emmahodcroft commented 1 year ago

Hey Richard! I used this today to show the variant dynamics of 2021 and it was really handy. I know it's not perfect and you're doing other stuff (totally get that), but I just wanted to let you know, I still think this is really cool & useful!

richardgoater commented 1 year ago

Hey Emma! Thanks so much for letting me know, I'm so happy that you could make use of it!

I'd really love to finish this and take a look at the per variant charts but I'm still finding that I need to protect my physical health when it comes to coding. Maybe I can make use of one of our abundant bank holidays in the next month :)

Is there anything that you'd like to add/change on the current version? I'd probably like to add a tooltip for the slider, and apply it to the other charts as mentioned before 👍🏽

richardgoater commented 1 year ago

Hi folks! Hope you're doing well. I've found that it's quite nice to do some coding while watching test cricket 😊

So I think I've made some good progress but there might still be a few demons still in it. I had a bit of trouble with the toolbar on the variants page and needed to try a few different UI patterns to get it going on mobile and smaller screens.

I've tried to float the toolbar to the right to make it less distracting overall, and a nice small change is closing the sidebar by default on mobile so we don't have to scroll past it to find the charts. The biggest change is the "bottom sheet" pattern on mobile. It gives space for both controls on the variant page, and allows them to be hidden when not in use:

but we still have an issue when the controls don't fit, I hope to find a solution for this:

I might need some help with the translation (great addition btw!) and typing issues, but let me know what you think 😄

richardgoater commented 1 year ago

Switched to golf now and I've added a quick fix for the wrapping issue above :)

emmahodcroft commented 1 year ago

Thank you @richardgoater - this is so cool!! Am just trying out the "flip up" toolbar in phone dimensions on my browser and it seems to work really well!

Agree that closing the sidebar by default is probably best behaviour for mobiles, since otherwise you scroll for ages to find the charts. For me, I didn't see this happening in my fake phone-browser, but I can use BrowserStack to test more 'realistically'.

I'm not sure I fully understand by

but we still have an issue when the controls don't fit, I hope to find a solution for this:

But maybe this was what you fixed already? 🙃

Anyway, this is really cool Richard - thank you for continuing to come back to this. It's sooooo useful for cases (to get rid of giant omicron humps!) I can't wait to see it live!

I've pinged @ivan-aksamentov in hopes he can take a look when time permits in the near future 😁

(PS - Sorry for springing the translations on you! I keep meaning to do a tweet but it seems to keep sinking to the bottom of the to-do each day.. Will aim to get that out soon!)

richardgoater commented 1 year ago

Thanks so much @emmahodcroft ! I'm sorry I left it for so long. The translation feature wouldn't affect this PR if it had been done sooner so no need to apologise :)

The closed sidebar should happen when you open the page in a window smaller than 768px, and it won't change on resize currently. Maybe you can see it in the fake phone-browser by refreshing?

Yes, I have put something quick in for that issue, I don't think it's optimal but it should only affect portrait tablets of a certain size.

Unfortunately I've just noticed that something is affecting the tooltips on mobile, they don't seem to be working now. Not sure why so I'll have to look into it more :/

richardgoater commented 1 year ago

Think I managed to fix that problem. I'm aware that the selection of the zoom area doesn't work on mobile, but at least the slider is available.

ivan-aksamentov commented 1 year ago

I noticed that there might be a mistake in the slider's range:

Repro:

narrow down the date interval using the slider
drag it back to the full range using the slider
click "Reset"

Observed: the plot range readjusts (extends) after clicked "Reset"

Expected: I'd expect that dragging the range back to full already resets the range and in this case the "Reset" button should have no effect.

Perhaps there's a rounding and/or off-by-one error in the range calculation?

We need to also ensure that the slider has the range that is union (the widest) of ranges for all the plots on different pages and even for different countries and variants, in case they happen to be different.

richardgoater commented 1 year ago

Thanks @ivan-aksamentov, very well spotted. The initial range isn't very sophisticated atm so I should look into it a bit more

richardgoater commented 1 year ago

Hey folks, I think I've solved the issue with the range changing when resetting the slider. Additionally I fixed some minor issues when zooming in close, so the charts won't go blank when the sliders are touching and there won't be multiple ticks of the same month showing.

Some misc. things: using the slider with the keyboard should look correct now, and the active style of the reset button should be completely visible.

hodcroftlab / covariants

Brush zooming #344