Extract RechartsWrapper to its own component

PavelVanecek commented 2 weeks ago

Description

I want to use hooks and dispatch here later so it has to be a functional component.

There are charts that are not using generateCategoricalChart, and they have almost the same component and could reuse this one too. With one difference, aria role. This uses region the other charts use application, which one is correct? @julianna-ciq what would be the correct labelling? I was studying the spec but as far as I can tell, neither is a good fit for interactive charts.

Motivation and Context

Use redux instead of element cloning.

How Has This Been Tested?

npm test

Screenshots (if appropriate):

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[ ] I have added a storybook story or extended an existing story to show my changes

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.60%. Comparing base (49dd104) to head (9245a77).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## 3.x #4545 +/- ## ======================================= Coverage 95.59% 95.60% ======================================= Files 116 117 +1 Lines 22430 22462 +32 Branches 3080 3081 +1 ======================================= + Hits 21443 21475 +32 Misses 981 981 Partials 6 6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ckifer commented 2 weeks ago

Tagging @julianna-langston as well, I think she has two accounts

julianna-langston commented 1 week ago

With one difference, aria role. This uses region the other charts use application, which one is correct?

The chart should have 1 role, role="application". It should be at the top, in whatever is the container of the chart.

When it comes to roles, the only thing we care about is what a screen reader thinks about it. When a screen reader sees role="application", it knows that thing, and all of its descendants, are interactive. In other words, we use role="application" because it makes the element interactive.

Now, our charts are only interactive sometimes. If accessibilityLayer={false} (current default, won't be default in 3.x) and there's a <Tooltip />, then it's interactive. Otherwise, it's not interactive. In that case, philosophically it'd be better to have role="img" at the container level, but I think that'll be difficult to implement, and the impact to a user is minimal.

Let me know if you'd like me to elaborate on how exactly role="application" works for screen readers.

PavelVanecek commented 1 week ago

Thank you @julianna-langston , I made the change in new PR.

I think it would be valuable to have chart behaviour from a11y point of view written down in a wiki in the repository, and we would use it twice:

When making changes or adding tests, we would refer to the document to see what is the correct behaviour
When choosing libraries, devs could check out the document and see what exactly is supported or not

recharts / recharts