Add chart component #4301
Add chart component #4301
Conversation
andysellick
force-pushed
the
add-chart-component
branch
from
5c9067a to
763f019
Compare
andysellick
force-pushed
the
add-chart-component
branch
from
c8dec1d to
939dae8
Compare
andysellick
changed the title
|
Also, I appreciate that this was a difficult component to take on, so thanks for doing this 👍 |
app/views/govuk_publishing_components/components/_chart.html.erb
Outdated
Show resolved
Hide resolved
app/views/govuk_publishing_components/components/docs/chart.yml
Outdated
Show resolved
Hide resolved
Yeah, the axis labels disappearing as the graph gets narrower is a bit unhelpful, although this seems to be the behaviour of the graph so I think we're stuck with it. I think the problem might be that (certainly with some data) it wouldn't be possible to fit the labels (certainly for the Y axis) in with the graph on mobile - imagine if the Y scale was On the point about the tooltip, I've just pushed a commit to switch to using HTML tooltips (instead of SVG). It seems now that it's much easier to dismiss the tooltip by tapping elsewhere on the page, so hopefully this has improved things. |
There was a problem hiding this comment.
Thanks @andysellick - the experience on mobile is a lot better with the new tooltips.
Code wise everything looks good 👍 so I would approve the PR on the basis of code, but had a few questions:
- It seems that you can't right click the element - is that intentional?
- The charts seem to have an
aria-labelthat isA chart.- is the lack of customisation for this intentional? The table has an aria label as well. - The HTML seems to have
col"attributes in it, which isn't a valid attribute - is this a chartkick thing we can ignore, or a bug? - The ONS charts fallback to an image when JS is disabled, so that may be worth noting for the future improvements to this component.
| assert_select ".govuk-table__cell--numeric", 26 | ||
| assert_select ".govuk-table__header", 14 | ||
| assert_select "td:first", text: "5" | ||
| assert_select "td:last", text: "121" |
There was a problem hiding this comment.
Just wondering for these tests if we should be testing data points in the middle as well (I guess it's better to be overly safe when it comes to data)
There was a problem hiding this comment.
Sure, have added.
| table_id = "table-id-#{SecureRandom.hex(4)}" | ||
|
|
||
| require "chartkick" | ||
| Chartkick.options[:html] = '<div id="%{id}"><noscript><p class="govuk-body">Our charts are built using JavaScript but all the data is also available in the table below.</p></noscript></div>' |
There was a problem hiding this comment.
Also, does this text need editorial approval?
There was a problem hiding this comment.
Possibly. If it's alright with you I think we'll look at that later when we're reviewing the pages as a whole.
It's not intentional from us, but appears to be intentional in Google charts, see this stack overflow for info: https://stackoverflow.com/questions/20266832/why-google-charts-are-without-right-click-menu
This is generated automatically by Google charts. It doesn't seem to be possible to directly customise this text - there are some complicated JS solutions online, which I'm hesitant to implement at this point as I think there's not much more helpful information we can provide at this level - the chart should already have a title, and an accessible description. Interestingly while digging into this I realised the component has two copies of the table - the one that we create inside the details component, and another one that Google charts automatically creates and appends after the chart. It's visually hidden, so still readable by screenreaders. I'll make a note to see if I can hide it somehow.
I'm not sure, where are these exactly?
Noted, thanks. |
|
@andysellick No worries - thanks for looking into those. I don't think the |
andysellick
force-pushed
the
add-chart-component
branch
from
6a778c4 to
dd89779
Compare
andysellick
force-pushed
the
add-chart-component
branch
from
dd89779 to
1d41ea9
Compare
andysellick
force-pushed
the
add-chart-component
branch
from
1d41ea9 to
e090912
Compare
andysellick
force-pushed
the
add-chart-component
branch
from
e090912 to
4aa9882
Compare
- copy of code from content-data-admin, with the following adjustments - added explicit `require "chartkick"` in template to prevent error - added template calls to include required stylesheets for chart, table and details component - modified output of keys to simply output key, previously was doing `key.to_formatted_s(:table_format)` expecting a date, to convert it into a slightly more human readable format, but this was erroring, so simply outputting the key, adjusted test to match - renamed class from `app-c-` to `gem-c-` - replaced instances of 14 font to 16, in line with Design System guidance - reordered some of the Sass so `@include` lines come at the end of declarations, to avoid Sass compilation warnings - modified test slightly to fit format of the gem tests
- add and style axes labels and legends - remove some specific styles that were overriding normal table styles - simplify CSS, chart height appeared to be duplicated - increase point size - remove specifics for line colour and style, let the chart decide
- clean up, reorder and remove redundant code - fix table vertical option - add heading element - minimise and simplify component options - add option for a chart overview description - add option to hide legend - remove unnecessary use of govspeak to wrap a visually hidden element - remove unneed styles - add tests, expand and clean up documentation
- using Google charts instead - chart.js is the default and produces nice responsive canvas graphs but provides far fewer options for customising important things like the font size and style of axes
- tooltip now uses the HTML option, which for some reason means it's easier to dismiss on mobile by tapping elsewhere on the page - also improve the styling, both in terms of code and appearance
- move accessibility text into the YML file - clean up the doc file a little and mark component as experimental
- Google charts automatically appends a tabular version of the graph data in a visually hidden element after the chart, but this is redundant and a bit confusing as we already have a table of the data displayed underneath the chart for all users - there seems to be no configuration option for controlling this automatic table so I've used some slightly cludgy CSS to display none it, thereby hiding it from all users including screen readers
- adds the shared helper to control margin bottom - adds the component wrapper helper because I initially remembered it wrong and thought it did margin bottom but then remembered it was the shared helper that did that but actually having the component wrapper helper on this component is no bad thing - adds an option for a download link
- smoothing distorts the graph by implying that change between data points is smooth when it isn't
andysellick
force-pushed
the
add-chart-component
branch
from
0583c75 to
061745c
Compare
What
Adds a chart component.
Based on a component from
content-data-adminoriginally added here: alphagov/content-data-admin#9TODO:
Why
Visual Changes
New component:
Trello card: https://trello.com/c/gvLYQCUp/57-build-statistics-charts