performance: replace recharts charting lib with Chart.js #12952
Conversation
github-actions
bot
added
dependencies 📦
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Screen.Recording.2024-05-15.at.09.47.34.mov@nhsz Noticing this when playing around. It's a fairly unnatural approach for someone to resize a window like that, but also noticing it starts getting cramped on mobile:
|
There was a problem hiding this comment.
Having a couple issues here on desktop... In the Netlify preview, the homepage loads correctly showing the 30-day charts, but clicking on the 90-day button for any of them gives me the dreaded Application error: a client-side exception has occurred (see the browser console for more information).
Locally I do not get this, and I am able to toggle between them. I then notice a difference in our transition between 30 and 90 days, where it seems to only be showing 30 days worth of data during the transition (in both directions) leaving a dead space in the chart during the transition.
Screen.Recording.2024-05-16.at.11.47.24.mov
Is there a way to maintain all 90 days in the state while this transitions?
There was a problem hiding this comment.
Noting the same @nhsz. Locally (dev) its working fine but on the preview deploy it is crashing.
|
|
||
| const chartOptions = { | ||
| // chart styles | ||
| barThickness: 38, |
There was a problem hiding this comment.
Wondering if we could pass a responsive Chakra token here to make the width responsive... or perhaps we could use a maxBarThickness here to cap it on Mobile.
| barThickness: 38, | ||
| borderRadius: 4, | ||
| aspectRatio: 1.1, | ||
| responsive: true, |
There was a problem hiding this comment.
Odd that the chart doesn't re-expand if you shrink then widen the device window... Would think this would take care of it, and the Box it's contained within should be responsive... Will keep an eye out for a possible patch
|
https://deploy-preview-12952--ethereumorg.netlify.app/en/energy-consumption From Safari: From Brave: Screen.Recording.2024-06-05.at.11.16.26.movScreen.Recording.2024-06-05.at.11.22.29.movCleared cache and tried this out... seeing some strange behavior on the engergy consumption chart where it glitches by loading/animating the chart twice, and seems to not be placing them in the correct sorting order. These adjust a little when the window size is adjusted. |
There was a problem hiding this comment.
@nhsz the energy consumption chart is looking better now even though it breaks when resized and the columns start to overlap a bit. I wouldn't consider this last issue as a blocker, as I doubt most users play with the window size.
One thing that could be discussed with the designers (@nloureiro @konopkja) is the height of the chart on mobile. I see that it is ~550px on the preview vs ~350px in prod. Not sure how easy it is to set it to something similar to prod.
====
Regarding the home page charts, I still get the error crashing the app in the preview deploy. I know that we saw that you didn't have this error yesterday but I have double checked this in different browsers (FF, chrome, brave, chrome android) and I still get the error. I'll try to debug a bit this today.
… add rtl support, improve type safety, and add story
Fix charts using chart.js
|
Great, the height looks good on the energy chart now. Responding well between screen sizes, I'm just still getting the double-load animation bug. I thought this was remedied with #13212 but maybe not? @pettinarip @nhsz Do you see this as a blocker? The homepage charts are working as expected again aside from the slight difference in how it animates. No more crashes or errors when switching between 30/90-day ranges. Small nice-to-have regarding the chart animation, right now it's anchored to the left which is a little strange since it's the right edge of the chart data that remains the same "day" (today)... would be nice if we could flip the animation anchor point to be the right edge instead. |
There is a hydration issue that is not related to this PR (its a pre-existing bug). We are going to tackle that on a separate PR. |
There was a problem hiding this comment.
LGTM @nhsz!
@wackerow the nice to have that you mentioned could be explored and added on a separate PR (to not block this long waiting PR), or even we could deactivate it if it doesn't convince us. Open for that as well. LMK what do you think or if you already have an idea about how we could achieve this.
This PR replaces the
rechartscharting lib withChart.js, with the goal of providing a lighter alternative and reduce the bundle size.We currently have 2 types of charts on the site, the
/StatsBoxGrid/GridItemon the homepage andEnergyConsumptionChartDescription
src/components/StatsBoxGrid/GridItem.tsxwith Chart.js implementationsrc/components/EnergyConsumptionChart.tsxwith Chart.js implementationuseIsClientcustom hookconstants.tscharts.tsto/lib/utilsrechartschart.jschartjs-plugin-datalabels(required plugin for custom datalabels)react-chartjs-2(React Chart.js wrapper)Charts location
StatsBoxGridEnergyConsumptionChartPreviews
Parsed size comparison
Chart.js (152.54kb)
recharts (271.81kb)
Reduction of 43.88%
Note
Found a small glitch on the StatsBox chart (don't think it's a blocker) that happens from time to time on some browsers like Chrome/Brave. Works ok on Firefox & Safari. From the comments on github, seems to be a browser issue and can't be fixed on the lib
Check the comments below: