Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/VictoryBar & Bar components ariaLabel and tabIndex [ready] #1687

Merged
merged 2 commits into from
Sep 29, 2020

Conversation

ljones87
Copy link
Contributor

@ljones87 ljones87 commented Sep 24, 2020

This PR adds the following props to the VictoryBar component:
ariaLabel: string or callback
tabindex: number or callback

Bars can now be tabbed individually with a unique aria-label applied to each (using the props available in the CallbackArgs:

export interface CallbackArgs {
  active: boolean;
  datum: any;
  horizontal: boolean;
  index: number;
  x: number;
  y: number;
  scale?: {
    x?: D3Scale;
    y?: D3Scale;
  };
}

This new feature is demo-able by running this branch, visiting localhost:3000 and clicking the new AccessibilityDemo link (both ts and standard js)
Screen Shot 2020-09-24 at 4 53 33 PM
Screen Shot 2020-09-24 at 5 01 58 PM

From there you can click anywhere on the bar graph space, hit tab, and you should see the focus move from one bar to the next. Inspecting the elements via devtools should show the aria-labels on each path component.

Todo:

  • tests
  • see if documentation needs updating - will do this at the end
  • fix tab formatting/indentation

I would like to PR a change like this for each of the component packages, adding the newly accessible chart (if it isn't already) to the accessibility demo.

@ljones87 ljones87 marked this pull request as draft September 24, 2020 23:00
demo/js/components/accessibility-demo.js Outdated Show resolved Hide resolved
packages/victory-bar/src/bar.js Show resolved Hide resolved
packages/victory-bar/src/bar.js Show resolved Hide resolved
packages/victory-core/src/index.d.ts Outdated Show resolved Hide resolved
@boygirl
Copy link
Contributor

boygirl commented Sep 24, 2020

@ljones87 this is looking great! I see that you also pulled the tabIndex and ariaLabel props up to VictoryBar. This makes it a bit more convenient for users, which I like, but it's also valid in Victory to do something like this:

<VictoryBar
  data={myData}
  dataComponent={<Bar ariaLabel={({ datum }) => `x: ${datum.x}`} />}
/>

I'm happy for you to carry the pattern you're putting in place, though, since it is a bit more convenient and potentially less confusing

@boygirl
Copy link
Contributor

boygirl commented Sep 24, 2020

I'm happy for these to all come in as granular PRs, but let's save the docs updates until the end, since they deploy on merges to main. I'd rather have the feature finalized first.

@ljones87
Copy link
Contributor Author

Hopefully I'll be able to start chunking a few together at a time, in whatever amount makes sense to still keep the PR's easily reviewable

@ljones87 ljones87 changed the title [WIP] Feature/VictoryBar & Bar components ariaLabel and tabIndex Feature/VictoryBar & Bar components ariaLabel and tabIndex [ready] Sep 25, 2020
@ljones87 ljones87 marked this pull request as ready for review September 25, 2020 19:49
@boygirl
Copy link
Contributor

boygirl commented Sep 28, 2020

@ljones87 This is looking good, but please change from ariaLabel to aria-label for consistency

@boygirl
Copy link
Contributor

boygirl commented Sep 28, 2020

@ljones87 Sorry for the back and forth, actually, since ariaLabel has different behavior from the standard aria-label prop (accepting a function), let's leave it camelCased to differentiate it.

@boygirl boygirl changed the base branch from main to feature/aria-label September 28, 2020 20:56
Copy link
Contributor

@kylecesmat kylecesmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! @boygirl had good feedback, and it was nicely addressed. Lets merge :shipit:

@ljones87 ljones87 merged commit fe738aa into feature/aria-label Sep 29, 2020
@ljones87 ljones87 deleted the feature/aria-label-bar branch September 29, 2020 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants