-
-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to Chart.js #963
base: main
Are you sure you want to change the base?
Move to Chart.js #963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
It's better now, when we don't start y-axis from 0.
-
Y-axis labels formatting fails to show the decimal part (here) Is it a quick fix?
-
Were the zoom/export buttons a feature of Highcharts? They've gone crazy on mobile charts.
We can make them start from 0 easily enough.
Of that's interesting. I explicitly dropped the decimal places but that doesn't work well for this particular chart, because we no zoom them in. Hmm... will have a think.
Yeah they were, and not now. So using some "magic numbers" as I said above. Haven't tested them thoroughly except on my own Prox Max iPhone., but guess not on your phone. Need to do some more work on this it seems... |
Hmmm starting at zero might be the easiest to solve 2, but you said you liked it in 1. Will go with that for now and can revist. |
No, don't change this. Keep it as it is. |
Fixed the Zoom/export buttons on mobile. |
cdf.show(); | ||
legend: { | ||
display: true, | ||
position: 'bottom', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but a general observation:
The legend on the top immediately informs viewers about the categories before they look at the chart. VS when it's in the bottom, it interrupts the flow (to scroll and find), makes confused.
position: 'bottom', | |
position: 'top', |
Also timeseries.
And I know we repeat them for every chart, but it's impossible to remember the colours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes it a little trickier with the extra buttons that overlay the chart. Not impossible but a little trickier. Will leave this comment open but might merge without that. Will see...
@tunetheweb initial accessibility thoughts (only looking at the implementation in your example and the accessibility documentation on their website):
From an accessibility POV I feel that something using SVG would give us more options to make it accessible, and also give more flexibility to the users. Can investigate more tomorrow 🤓 |
Thanks for looking! Yeah it really does remove all interactivity from AT and basically treats it as an image (I even added Outside of your work on the Tech Report, that's basically the same as the status quo, so I saw it more s a missed future opportunity. Though I hadn't considered font-size changes or other CSS overrides which is a good point.
I spent a good bit of time on this and there's not great plugins. This one looked most promising, but it broke our graphs. It's a pre-release plugin so probably not that surprising.
The markers can be styled. Just added it to show on hover and re-staged.
For simple line graphs maybe. But think the fact charting libraries exist show it's not that simple. Plus we have more complex histograms when you choose one month to look at: |
amCharts looks like another option with good accessibility options and a freeware option with attribution. Will look into that... |
Our current charting solution (Highcharts) is getting more and more aggressive about requiring a licence and not offering one for non-profit, open-source projects like ours. The latest version makes this even clearer.
It does not appear to be possible to purchase a licence that would cover all potential contributors to the project. And even if we could, usage of such a restrictive licence goes against our open-source ethos and attempt to provide a best practice for others to learn from.
I've therefore looked into Chart.js and think it's a viable alternative, in no-small part due to it's open-source nature and permissive MIT licence.
This PR switches to Chart.js for Timeseries and Histograms (new CWV Tech Report to follow if this is agree).
I've staged this PR here so you can see what it looks like:
https://technology-report-dot-httparchive.uk.r.appspot.com/reports/state-of-the-web
(really must sort out a proper staging env!)
Switching has some pluses and negative:
Great!
However, there are some downsides:
Have a play with the staging link and would love some feedback on the approach and the PR but overall I think this is pretty good to merge.