demo/a11y-docs-demos #21112
demo/a11y-docs-demos #21112
Conversation
Visual test results - Differences foundFound 4 diffing sample(s). Please review the differences. Samples changed
|
File size comparisonNo differences found |
There was a problem hiding this comment.
A lot of great demos!
I haven't tested screen reader because I have little expiernce in using it, but please let me know if I should review again with this type of testing included in my review.
Some comments are in one place but are more general and apply to other similar cases as well (e.g. templating; linking API options in the descriptions).
Maybe this should be a different task on its own, but in general we are using the same CSS options for tabels over and over again in the samples and demos - it would be nice to DRY this.
| <figure class="highcharts-figure"> | ||
| <div id="container"></div> | ||
| <p class="highcharts-description">The following chart demonstrates a chart showing the use of the <code>dashStyle</code> option. | ||
| The chart has two series where the first series is using the <code>dashStyle: 'longdash'</code> and the second series is using the <code>dashStyle: 'shortdot'</code>. |
There was a problem hiding this comment.
If the demo will be linked in API this might be redundant, but maybe the API options could be linked to the API?
There was a problem hiding this comment.
It might be linked in the API, and also the option is linked in the documentation where the demo is used. So it might be redundant 🤔 But if someone is seeing the demo without any context, then it might be useful
| @@ -0,0 +1,23 @@ | |||
| Highcharts.chart('container', { | |||
There was a problem hiding this comment.
The chart title ("Chart title") could be changed to something better or hidden. Same for the series names - maybe something relevant to the dashStyle option that they are using?
| @@ -26,12 +26,18 @@ Highcharts.chart('container', { | |||
| }, | |||
| series: [{ | |||
| name: 'John', | |||
| data: [5, 3, 4, 7, 2] | |||
| data: [5, 3, 4, 7, 2], | |||
| borderWidth: 2, | |||
There was a problem hiding this comment.
Common or repeated series options might look better in plotOptions.
| max-width: 800px; | ||
| height: 400px; | ||
| #container { | ||
| height: 420px; |
There was a problem hiding this comment.
Not sure if this is a real problem, but it looks like the lack of min-width style prevents the mobile preview in the utils.
There was a problem hiding this comment.
The mode switch is also not working in the iPhone simulator, when opening http://localhost:3030/samples/view?path=highcharts/accessibility/accessible-switch-theme-one
There was a problem hiding this comment.
Thanks, will take a look!
There was a problem hiding this comment.
Thanks for pointing that out! I had a extra closing <div>-element after the radio buttons that broke the mobile-preview 😅
| @@ -3,4 +3,4 @@ | |||
| <script src="https://code.highcharts.com/modules/export-data.js"></script> | |||
| <script src="https://code.highcharts.com/modules/accessibility.js"></script> | |||
| <div id="container"></div> | |||
| <button id="stop">Stop updating live data</button> | |||
| <button id="toggle-announce">Stop updating live data</button> | |||
There was a problem hiding this comment.
I believe buttons need some classes to pick up the styling from Bootstrap like in projection explorer or a custom styling could be used like in maps overview.
| series.points.length > 20 | ||
| ); | ||
| }, 2000); | ||
| startUpdatingData(); |
There was a problem hiding this comment.
load: startUpdatingData also works and matches line No. 26.
| color: '#3A3691', | ||
| accessibility: { | ||
| description: 'Emma walked the most in total during the week with ' + | ||
| '75739 steps.', |
There was a problem hiding this comment.
If the total could be calculated instead of hard-coded using templating then this could be a nice flex for using templating.
| <figure class="highcharts-figure"> | ||
| <div id="container"></div> | ||
| <p class="highcharts-description"> | ||
| The following chart is demonstrating a very simple configuration, without proper context. |
There was a problem hiding this comment.
Is this a demo showing what not to do? If yes, this might not be clearly described.
| <div id="container"></div> | ||
| <p class="highcharts-description">The following chart demonstrates some Accessibility features of Highcharts, including use of the <code>pointDescriptionEnabledThreshold</code> option. | ||
| The <code>pointDescriptionEnabledThreshold</code> is set to 1 in this demo, which means the individual points are are not exposed to a screen reader if a series contains more than one point in this case. | ||
| This might be useful for larger series and is set by default to 200 for a chart.</p> |
There was a problem hiding this comment.
"This might be useful for larger series and is set by default to 200 for a chart."
The wording suggests the limit is for a chart, so the total number of points from all chart series. How about "This is useful for large series and is set by default to 200." ?
|
|
||
| series: [{ | ||
| data: [1, 3, 2, 4, 5, 4, 6, 2, 3, 5, 6], | ||
| dashStyle: 'longdash' |
There was a problem hiding this comment.
Not sure if this is related to the second PR or a side note. When I trigger range change animation by deactivating the second series, the animation on the right side is very intense even though I have reduced animation active.
There was a problem hiding this comment.
Thanks for the input! Noted down for research.
| @@ -4,6 +4,6 @@ | |||
| <script src="https://code.highcharts.com/themes/high-contrast-light.js"></script> | |||
|
|
|||
| <figure class ="highcharts-figure"> | |||
| <div id="container-area" class="container"></div> | |||
There was a problem hiding this comment.
Both pattern fill demos do not differentiate arearange series by pattern style like the other series types. This is problematic for color blindness.
| max-width: 800px; | ||
| height: 400px; | ||
| #container { | ||
| height: 420px; |
There was a problem hiding this comment.
The mode switch is also not working in the iPhone simulator, when opening http://localhost:3030/samples/view?path=highcharts/accessibility/accessible-switch-theme-one
| data: [3, 4, 4, 2, 5] | ||
| data: [3, 4, 4, 2, 5], | ||
| borderWidth: 2, | ||
| borderColor: '#000000' | ||
| }] | ||
| }); |
There was a problem hiding this comment.
In case this should be possible, I am not able to enter the table with keyboard or screen reader.
| const chart = Highcharts.charts[0], | ||
| series = chart.series[0]; | ||
| updateIntervalId = setInterval(function () { | ||
| series.addPoint( |
There was a problem hiding this comment.
Some issues:
- Keyboard focus is not animated.
- After 20 points the first points get removed, which removes keyboard focus and I end up in the side panel.
There was a problem hiding this comment.
Thanks, this is noted down!
| <p class="highcharts-description">The following chart demonstrates some Accessibility features of Highcharts, including use of the <code>exposeAsGroupOnly</code> option. | ||
| The <code>exposeAsGroupOnly</code> is set to true in this demo for two of the series, which means that only the series element is exposed to a screen reader, not its points.</p> |
There was a problem hiding this comment.
I am able to enter the series group after focus and reach series points by ctrl + option + arrow down. Maybe the text needs some clarification that this is limited to the text but not keyboard navigation.
| }, | ||
| accessibility: { | ||
| series: { | ||
| pointDescriptionEnabledThreshold: 1 |
There was a problem hiding this comment.
This might be an error. I still get picture info for each point during navigation (in Safari).
| @@ -1,4 +1,5 @@ | |||
| <script src="https://code.highcharts.com/highcharts.js"></script> | |||
| <script src="https://code.highcharts.com/modules/exporting.js"></script> | |||
| <script src="https://code.highcharts.com/modules/accessibility.js"></script> | |||
There was a problem hiding this comment.
I am not able to reach the chart in Safari by keyboard.
Branch for upgrading and adding new demos to the Accessibility Documentation ( #21111 ).