Add new chart component options #4318
Conversation
andysellick
force-pushed
the
add-small-chart
branch
from
3ae4245
to
9e8a228
Compare
andysellick
force-pushed
the
add-small-chart
branch
from
62d6e71
to
d62c443
Compare
andysellick
changed the title
| enableInteractivity = false if minimal | ||
| textPosition = nil | ||
| textPosition = 'none' if minimal | ||
| valid_minimal = true |
There was a problem hiding this comment.
Would it be better to use rails validation here? E.g.
if minimal && !minimal_link
raise ArgumentError, "Minimal version must include a link"
endThere was a problem hiding this comment.
Good shout, thanks, have updated.
| @@ -75,77 +89,85 @@ | |||
| <% end %> | |||
|
|
|||
| <div class="gem-c-chart__chart" id="<%= chart_id %>"> | |||
There was a problem hiding this comment.
If the purpose of the minimal version is just for presentation, and not for conveying information, should it be hidden from the accessibility tree? E.g. give it a tabindex so that it can't be focused on, and use aria-hidden="true"?
If not, I think the focus style for the minimal version should be improved - for example the yellow is not visible around the whole chart container. Otherwise, I think it will be flagged as an accessibility issue.
Also, how will users know that this is a link? Will it be next to some content that implies so?
There was a problem hiding this comment.
Good shout on the first point - have added aria-hidden as suggested so now the only thing a screenreader should encounter is the link. Have also improved the focus style for the link to make it look better.
How will users know this is a link? That's an excellent question and I'm not sure of the answer. In the designs we have they're directly underneath some headings, which themselves are also links, so this being a link as well is kind of a backup (although that itself introduces a duplicate link, which isn't great).
I'm left wondering if minimal should be a link at all, but the link is needed to allow users to go to the full graph and table of data - it might as well just be an image if it isn't a link, but we decided to generate a chart otherwise we have to keep updating images every time the chart they link to changes. A bit of a rabbit hole in general.
| - 82 | ||
| - 118 | ||
| - 85 | ||
| - 80 |
There was a problem hiding this comment.
Nitpick: no empty line at the end of the file
andysellick
force-pushed
the
add-small-chart
branch
from
d62c443
to
03f2ac9
Compare
- currently hard coded as the chart isn't responsive - quite difficult to write a test for this as it's applied as an inline style
- use an Openstruct to persist a value between multiple instances of the component to make the external script for Google charts only be called once - perhaps unsurprisingly this doesn't make any difference to the page weight in Chrome, presumably because Chrome is smart enough to realise it's already loaded that script if there's more than one chart - however it feels tidier to limit it this way, particularly for browsers that may not be so clever
andysellick
force-pushed
the
add-small-chart
branch
from
c503857
to
357afbd
Compare
| @@ -92,14 +92,15 @@ | |||
| } %> | |||
| <% end %> | |||
|
|
|||
| <div class="gem-c-chart__chart" id="<%= chart_id %>"> | |||
| <% aria_attributes = { hidden: true } if minimal %> | |||
There was a problem hiding this comment.
I think if we have aria-hidden i don't think it can be keyboard focusable as well, as the component guide is flagging this as an issue. Happy for you to pick between either having focus or having aria-hidden.
There was a problem hiding this comment.
Argh, good spot, thanks. Think I've fixed this now - since the chart is just a link, I've included some logic to simply not render the visually hidden text (it's the thing that contained a link).
andysellick
force-pushed
the
add-small-chart
branch
from
be71211
to
0babe99
Compare
|
@AshGDS are you happy to approve the visual changes? |
- improve focus state around link to have a complete border and be more visible - use aria hidden on the graph when in minimal mode
andysellick
force-pushed
the
add-small-chart
branch
from
0babe99
to
cc9ccf5
Compare
What
Adds some options to the chart component:
I've started going down a bit of a rabbit hole with this one, so I've raised #4322 as an issue to record my concerns rather than try to address them here and spiral the scope out of control.
Why
Needed for a page where the graphs are going to be small and link to another page.
Visual Changes
Minimal version:
Minimal version with a smaller height:
Trello card: https://trello.com/c/DFT05NWN/97-implement-small-graphs-on-main-landing-page