Skip to content
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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Move to Chart.js #963

wants to merge 24 commits into from

Conversation

tunetheweb
Copy link
Member

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!

  • Free! For us and others
  • Similar structure with JSON config
  • Very extensible, which allows making up for missing functionality.
  • Actively being maintained
  • Active community providing additional plugins and functionality
  • Similar size and performance.

However, there are some downsides:

  • We lose our minimap beneath the charts
  • Labels aren't as configurable
  • Changelog timestamps required some custom code and even then doesn't support hovers.
  • The charts are not keyboard accessible. Neither are ours on the main site though we did want to chanfge that (see Investigate accessibility plugin for HighCharts #765). @sarahfossheim did make the new Tech Report keyboard accessible using that extra functionality, so we'd lose that 😔 Now we do have tables of the data beneath which makes the data itself accessible, but it's still a shame. I tried using a plugin to provide this but it messed up our charts. Sarah, would love your opinion here?
  • No zoom or export buttons by default. It uses some magic numbers to get the buttons to overlay the chart. It works for now, but might want to revisit this later to improve it.

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.

src/js/histogram.js Fixed Show fixed Hide fixed
src/js/histogram.js Fixed Show fixed Hide fixed
src/js/histogram.js Fixed Show fixed Hide fixed
src/js/histogram.js Fixed Show fixed Hide fixed
src/js/histogram.js Fixed Show fixed Hide fixed
src/js/timeseries.js Fixed Show fixed Hide fixed
src/js/timeseries.js Fixed Show fixed Hide fixed
src/js/timeseries.js Fixed Show fixed Hide fixed
src/js/timeseries.js Fixed Show fixed Hide fixed
src/js/timeseries.js Fixed Show fixed Hide fixed
Copy link
Contributor

@max-ostapenko max-ostapenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It's better now, when we don't start y-axis from 0.

  2. Y-axis labels formatting fails to show the decimal part (here) Is it a quick fix?

  3. Were the zoom/export buttons a feature of Highcharts? They've gone crazy on mobile charts.

@tunetheweb
Copy link
Member Author

tunetheweb commented Dec 2, 2024

  1. It's better now, when we don't start y-axis from 0.

We can make them start from 0 easily enough.

  1. Y-axis labels formatting fails to show the decimal part (here) Is it a quick fix?

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.

  1. Were the zoom/export buttons a feature of Highcharts? They've gone crazy on mobile charts.

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...

@tunetheweb
Copy link
Member Author

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.

@max-ostapenko
Copy link
Contributor

max-ostapenko commented Dec 2, 2024

  1. It's better now, when we don't start y-axis from 0.
    We can make them start from 0 easily enough.

No, don't change this. Keep it as it is.

@tunetheweb
Copy link
Member Author

OK reverted that and set precision to be 0 so it only shows whole number tick marks to avoid duplicating them when formatted to whole numbers:

image

@tunetheweb
Copy link
Member Author

Fixed the Zoom/export buttons on mobile.

cdf.show();
legend: {
display: true,
position: 'bottom',
Copy link
Contributor

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.

Suggested change
position: 'bottom',
position: 'top',

Also timeseries.

And I know we repeat them for every chart, but it's impossible to remember the colours.

Copy link
Member Author

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...

@sarahfossheim
Copy link
Collaborator

@tunetheweb initial accessibility thoughts (only looking at the implementation in your example and the accessibility documentation on their website):

  1. Since it uses canvas instead of SVG the data inside is no longer screen reader accessible. On the CWV tech report screen reader users can navigate the datapoints on the lines as well, and each line can have additional info. We can probably make the alt text slightly more useful (maybe something based on min, max, latest value, and trend for each line?), but between that and linking to a data table there's not much else that can be done AFAIK (I see there's mention of plugins for chart.js, can take a look if there's anything that could work there)

  2. Keyboard nav lacking as you mentioned. On the CWV tech charts left and right arrow can be used to go through the line chart and expose the tooltips that way.

  3. Another side effect of canvas is that it's less customisable by visitors. Not sure how common of a use case this is on httparchive, but theoretically speaking someone could customise the colors (eg. to achieve higher contrast) of the charts through custom stylesheets using a plugin (I use Stylebot to test). Eg .highcharts-line-series > path[stroke="#669E8E"] { stroke: black; stroke-width: 4px; } will make all the green lines on the CWV tech report thicker and black.

  4. More common use case could be increasing font-size. If the base font-size is increased by the user, text in the svg using relative units will scale up, the canvas text in the example doesn't.
    Canvas: plugin sets font-size to 2em, font-size on the regular elements on the page is scaled up, the canvas element still has 1em font size SVG: plugin sets font-size to 2em and stroke of one of the lines to 4px black, all chart elements are updated accordingly

  5. Not super familiar with chartjs, will need to look into if styling the markers and tooltips will still be possible, since that's another accessibility feature in highcharts we're losing:
    line chart with blue and purple line, a specific point is hovered, showing a circular marker on the blue line and a square on the purple one green line with circle as a marker, purple line with square as a marker

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 🤓

@tunetheweb
Copy link
Member Author

Thanks for looking!

Yeah it really does remove all interactivity from AT and basically treats it as an image (I even added role=img in this PR to make this clear). It's not great.

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 see there's mention of plugins for chart.js, can take a look if there's anything that could work there).

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.

Not super familiar with chartjs, will need to look into if styling the markers and tooltips will still be possible, since that's another accessibility feature in highcharts we're losing:

The markers can be styled. Just added it to show on hover and re-staged.
The tooltip text can be styled but not as much as highcharts (no HTML). They look more like the ones you're using anyway!

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 🤓

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:
https://technology-report-dot-httparchive.uk.r.appspot.com/reports/state-of-the-web?start=latest&view=list#bytesTotal

@tunetheweb
Copy link
Member Author

amCharts looks like another option with good accessibility options and a freeware option with attribution.

Will look into that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants