Making the SVG accessible

miichu commented 2 years ago

Hi!

When I run tests with the ARC toolkit in Chrome, I get these accessibility warnings.

Screenshot from 2022-01-13 12-15-01

To solve these errors and warnings, I was wondering if it was possible to add the attributes as described in the screenshot. I wanted to open a PR, however, I couldn't find the SVG to make the edits to. 🤔

I'll add an example of a fully accessible SVG.

const Ellipse = ({
  fill,
  id,
  ...rest
}: SVGAttributes<SVGElement>): JSX.Element => (
  <svg
    viewBox="0 0 8 8"
    {...rest}
    role="img"
    focusable={false}
    aria-labelledby={id}
  >
    <title id={id} />
    <circle cx="4" cy="4" r="4" fill={fill} />
  </svg>
)

export default Ellipse

palerdot commented 2 years ago

Thanks for raising this issue.

The part of code where SVG is rendered can be seen here - https://github.com/palerdot/react-d3-speedometer/blob/438fa33e59ebfdfffb0db59753ec281419109b52/src/core/render/index.js#L61

Ideally, aria-labelledby for SVG should be configurable via a prop, for eg: svgAriaLabel, so that people can configure what text should be added for aria-labelledby. The default can be something like React Speedometer.

logan4473 commented 2 years ago

May I fix this issue?

palerdot commented 2 years ago

@logan4473

Feel free to raise a PR.

Please consider following pointers

There should be a propsvgAriaLabel to configure SVG text (The default value can be React d3 speedometer)
Other two attributes can be inlined - role=img, focusable=false

logan4473 commented 2 years ago

@palerdot

How should I add a prop svgAriaLabel to configure SVG text?

palerdot commented 2 years ago

Adding a new prop requires some involved changes and writing tests. I have outlined steps to add a new prop below.

Steps for adding prop

Add the new prop in DEFAULT_PROPS (add all accessibility props at the last, with comment explaining that following props are related to accessibility) - around https://github.com/palerdot/react-d3-speedometer/blob/master/src/core/config/index.js#L67
Make sure new prop is considered when configuring the config - around https://github.com/palerdot/react-d3-speedometer/blob/master/src/core/config/index.js#L140

Adding tests:

Please make sure to add a new test to verify if the aria-label is showing correctly. Example tests can be referred here - https://github.com/palerdot/react-d3-speedometer/blob/master/src/tests/index.js#L88. Also, refer to enzyme documentation.

Important:

Make sure to attach the screenshot of successful results after running tests with the ARC toolkit in Chrome (pls refer https://github.com/palerdot/react-d3-speedometer/issues/135#issue-1101617187 for more details)
Make sure to add a test case (Please refer above section)
Make sure existing tests don't break
Make sure the code is linted properly. Run yarn lintfix locally to lint the code as per this project requirements.

Miscellaneous:

Please allow edits from maintainers option for me to update docs if PR is in an acceptable state. Please refer - https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

PS: Since this is an involved change, I'm removing the good first issue tag

palerdot commented 2 years ago

SVG Accessibility is released in v1.0.2.

Example sandbox - https://codesandbox.io/s/magical-galileo-swf7e?file=/src/App.js

palerdot / react-d3-speedometer