Use intersection observer to render visible charts

[richardgoater]

Hey folks,

I tried working with ECharts but I don't like it much. The scrolling issue we spoke about notwithstanding, the API is not great IMO and notice how the cursor line is not visible because reasons? I also noticed that it didn't improve performance when resizing the page and waiting for all the charts to update.

I noticed you've been trying out limiting the initial charts but I'd like to throw my hat in the ring with this. Think it's possible to extend the margin so that e.g. the chart you're about to scroll to is already rendered, but I've smoothed it out with fading anyway.

If you're keen to remove Recharts I think this would still be a useful approach. My variant chart changes are going well with Recharts though, so I can promise to implement the "brush" zooming we had on the Sanger dashboard for both 😇

Look forward to your comments.

[vercel[bot]]

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

Name	Status	Preview	Updated
covariants	✅ Ready (Inspect)	Visit Preview	Aug 9, 2022 at 9:38AM (UTC)

[ivan-aksamentov]

Thanks Richard, cool stuff and feels very snappy!

In principle it works as is and is very useful already. It also did not require revamping the whole page layout, compared to the approach with react-window I considered, so the implementation is much simpler. I like it!

Would be nice to extend this to all other plot pages.

Do you think there is a way to move the intersection-observer hook a few steps up in the components tree, away from the plot component, and towards, let's say PlotCard or so? It would then make it easier to extend the infinite scrolling to other plots (there are 3 pages with different plots now) and might give a chance to test it with different plotting libraries, without duplicating the code. Basically, I thought separating the infinite scroll implementation from the plot implementation would be nice.

Not sure if covariants prod still works in IE and on iOS at this point, but in case it does, we could also add polyfill for intersection-observer, to make sure it does not break. I believe Next.js recommends that polyfills go to _app.tsx. So the snippet from the react-intersection-observer docs can go to there.

--

Update: I just reread the code and see that intersection-observer hook requires a ref, so that it will be quite awkward to move it up, if ref needs to be attached to the plot. But perhaps it can be on one of the parents?

[ivan-aksamentov]

Regarding echarts, I find their interface rather unusual indeed, it's not adapted for React at all.

But I was so excited to see how many features it has and how many different beautiful plots it can draw. Unfortunately, turned out canvas is not much faster than SVG and the startup is sometimes even slower. Disappointed here.

Basically why I tried echarts is: I thought they might be faster (no), more features for future use (yes, but unclear if we need them) and built-in zoom (yes, but turns out broken for our use case).

But it's probably more suitable to discuss echarts in the original PR https://github.com/hodcroftlab/covariants/pull/319.

[richardgoater]

Hi @ivan-aksamentov! Great to hear from you and thanks for your great comments.

Completely agree that generalising it would be great. I think it looks like we can move ChartContainerOuter, ChartContainerInner, and useResizeDetector into one component - ChartContainer? We can add the intersection observer here as well, but let me know if you don't like this kind of "super" component. We would then remove ResponsiveContainer from all charts and pass an explicit height as you can see in this PR.

Great idea to add the polyfill, I suppose the webpack dynamic snippet would work? I also see that the library supports a fallback value so I can add that too.

I think Echarts is well worth trying and it's a shame that there are so many issues. I'll leave it up to you to decide the way forward, I can continue experimenting on the Variants chart with Recharts just so we can see if the basic premise works.

[richardgoater]

Just thinking a bit more, we would need something like a render prop interface to pass the width and height if we're going to use a super component - is that ok? The tricky part is that the observed element needs to use the dimensions of the chart so it can smoothly fade in without causing the page to layout, and this also needs to be inside the resize detector. Hopefully there's a way to generalise all of this...

[emmahodcroft]

Thanks @richardgoater ! I am afraid the technical details are beyond me - I'll leave that to Ivan - however, I just tried this out and it's super cool! As you noted, we have enough data/charts now that loading is a real issue that we were trying to solve. I love how Per Country loads now - it's smooth as butter. I also tried it out on mobile (a real pinch-point) in comparison to current CoV - it's fantastic!

Very hopeful this can be a very cool way forward! 🙏

[richardgoater]

Thanks so much Emma! Yes I love the improvement on mobile! 🤳🏽

[richardgoater]

So I tried it and the variant charts are taking a few seconds to load on my machine, even in isolation 😢. Would say this is a worse experience and does not bode well for changing the functionality of those charts either. Maybe a new chart library would be needed after all.

edit: ooh - the built version is quite a bit better! Doesn't feel too bad although there is still a short delay in the page loading?

[emmahodcroft]

So I've just tried it out and I have to say, it seems pretty good to me! Again, on mobile in particular, the performance improvement is super noticeable and really smooth!

Ivan and I have discussed offline (well, online, but on on Github 😆 ) that probably the Per Variants page needs a bit of a rethink overall. It's a lot of data and probably too much. The trick is how to change this - it's easy to say 'just plot fewer countries' but for some charts that'll mean they plot empty! All of this to say, it's fine if this PR doesn't solve this entirely - we probably need to continue to find a better solution for Per Variants - but any improvements along that road would still be welcome!

[richardgoater]

cool, thank you! I will just add the polyfill in that case 😊

[richardgoater]

sorry, failing miserably at this. I wonder if you're ok to do it without top-level await? It might increase the bundle size a bit. I'm also struggling with the TS/warnings in FadeIn.tsx, I think they're quite visible in development.

[emmahodcroft]

@ivan-aksamentov's away for a few days, but I think he's best to weigh in here on the pros/cons of with/without top-level await. Or perhaps he could help out or have a suggestion? I'm afraid my level here is too low to be of any use or help! 😅

[richardgoater]

Sorry I've been off GH for the week, no worries at all :)

We can see that it will add a few KB that not many people will need: https://bundlephobia.com/package/intersection-observer@0.12.2.

We could also suggest that adding the fallback value will stop it from crashing and that's good enough, so no need to mess around with the polyfill

[ivan-aksamentov]

Alright, I've resolved remaining issues with polyfills, with typescript and eslint. In terms of code this should is ready to go. Please double check that things still work as expected and perhaps, check that IE and, especially, iOS still work.

If any of iOS, let's say starting from around 5 years ago, is broken, it will definitely raise some eyebrows when Emma tweets about Covariants next time, users navigate from Twitter on their iPhones and see a blank page or an error.

[richardgoater]

Thanks so much @ivan-aksamentov! The code changes are really cool. I think however the fade-in animation is no longer working? I can take a look at this tonight.

I'm also noticing that the variants page has 6 charts in view on a 4K monitor and I think it reduces the effectiveness of this approach, but hopefully it is still an improvement.

[ivan-aksamentov]

fade-in animation is no longer working

oops!

[ivan-aksamentov]

FadeIn should be fixed in the latest commit

[richardgoater]

Thanks again @ivan-aksamentov! Everything is working on my new work phone, a 2022 iPhone SE. I can't remember if you have Browserstack or not, but I don't think I have access to your account. I used the free trial and a few older iPhones were not loading at all 😬 I think the spread operator was a problem?

[ivan-aksamentov]

@richardgoater I tested on a few Apple devices back to iPhone 8/ iOS 11 and it works okay. Probably does not worth efforts going anywhere beyond 5 years old, especially that it's not an easy task to do.

@emmahodcroft If you are satisfied with the functionality, this is fine to merge.

[emmahodcroft]

I worked my way back to something from 2016 (I have trouble keeping up with Iphone orders/numbers/what the letters mean) and as long as one doesn't scroll too quickly initially it seems to work fine! The loading is a little slower, but that's life with old phones.

@richardgoater we do have a browserstack instance - I'll send you an invite! This is so cool, thank you so much!

I'll push the update from today, then aim to merge this in!

[richardgoater]

Ahh thanks so much @emmahodcroft, really appreciate it! 🙏🏽