fix: icon story accessibility

[jsomsanith]

What is the problem this PR is trying to solve?
Icon story is not accessible. This is only on the story as we can’t do anything to ensure icon accessibility, it’s more linked to the usage.
On decorative icons we should hide it via aria-hidden, and on icons that hold information, we should set a proper aria-label.

1. it’s a list but it only uses divs
2. story with label has decorative icons, they should be hidden to SR
3. story without labels icons hold information, they should have some text alternatives
4. stories with inline/block icon should have a text alternative

What is the chosen solution to this problem?

1. use ul/li
2. add aria-hidden on the icons
3. set key as aria-label
4. set a meaningful aria-label

As suggested by @domyen, a comment in Icon code has been added, giving advice to make it accessible when using it.

[kylesuss]

I know its a bit late now as this is merged, but I was sketching out an idea to see if we could make this more enforceable based upon your comments @jsomsanith :

const NonDecorativeIcon = ({ label, ...rest }) => (
  <Svg viewBox="0 0 1024 1024" width="20px" height="20px" aria-label={label} {...props}>
    <Path d={icons[icon]} />
  </Svg>
);

NonDecorativeIcon.propTypes = {
  label: PropTypes.string.isRequired,
};

const DecorativeIcon = props => (
  <Svg viewBox="0 0 1024 1024" width="20px" height="20px" aria-hidden {...props}>
    <Path d={icons[icon]} />
  </Svg>
);

const Icon = ({ hasExternalLabel, ...rest }) => {
  if (hasExternalLabel) {
    return <NonDecorativeIcon {...rest} />;
  }

  return <DecorativeIcon {...rest} />;
};

Icon.propTypes = {
  block: PropTypes.bool,
  // This is required so that we don't assume all icons are decorative.
  hasExternalLabel: PropTypes.bool.isRequired,
  icon: PropTypes.string.isRequired,
};

Icon.defaultProps = {
  block: false,
};

export default Icon;

@jsomsanith what do you think about something like this? We can enforce the accessibility attributes this way, but is it overkill?

cc @domyen

[domyen]

I think it replaces one set of attributes to remember with props. Even though it makes it somewhat easier to enforce I feel like going with the standard aria is simpler to remember.

…

On Mon, Jun 24, 2019 at 5:42 PM Kyle Suss ***@***.***> wrote: I know its a bit late now as this is merged, but I was sketching out an idea to see if we could make this more enforceable based upon your comments @jsomsanith <https://github.com/jsomsanith> : const NonDecorativeIcon = ({ label, ...rest }) => ( <Svg viewBox="0 0 1024 1024" width="20px" height="20px" aria-label={label} {...props}> <Path d={icons[icon]} /> </Svg> ); NonDecorativeIcon.propTypes = { label: PropTypes.string.isRequired, }; const DecorativeIcon = props => ( <Svg viewBox="0 0 1024 1024" width="20px" height="20px" aria-hidden {...props}> <Path d={icons[icon]} /> </Svg> ); const Icon = ({ hasExternalLabel, ...rest }) => { if (hasExternalLabel) { return <NonDecorativeIcon {...rest} />; } return <DecorativeIcon {...rest} />; }; Icon.propTypes = { // This is required so that we don't assume all icons are decorative. hasExternalLabel: PropTypes.bool.isRequired, icon: PropTypes.string.isRequired, }; export default Icon; @jsomsanith <https://github.com/jsomsanith> what do you think about something like this? We can enforce the accessibility attributes this way, but is it overkill? cc @domyen <https://github.com/domyen> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28?email_source=notifications&email_token=AACAJWILSEBH65GVS47VULDP4E5TLA5CNFSM4H3CL4U2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYOKE7I#issuecomment-505193085>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACAJWM3CIRNC37HWG42Q2TP4E5TLANCNFSM4H3CL4UQ> .

[jsomsanith]

Instead of having an Icon element that switch to Decorative or NonDecorative, I would prefer to expose only the DecorativeIcon and NonDecorativeIcon. So the dev must pick one depending on the purpose. Those 2 components would use the same Icon internal component passing the extra aria-label or aria-hidden from their own required props

[kylesuss]

I think the aria stuff is hard to remember if you don't even know to consider using it in the first place. Pretty easy to just leave it out at that point. That said, I don't love the verbose nature of requiring the hasExternalLabel prop either -- was just trying to think this through a bit in terms of how the library will be used. It does seem too easy to me to leave the accessibility stuff out. I also prefer having 1 Icon component (vs exposing both DecorativeIcon and NonDecorativeIcon), so maybe that means we have the easiest solution so far and we just have to rely on people checking the design system story docs before using a component.

Seems like a prime use case for docs mode w/ MDX 😉 I think I will use this as a testing ground for some of that documentation.

[kylesuss]

🚀 PR was released in v0.0.27 🚀