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

Take letter-spacing and font-size into consideration while rendering ticks #2898

Merged
merged 7 commits into from Aug 18, 2022

Conversation

saghan
Copy link
Contributor

@saghan saghan commented Jul 11, 2022

This is an attempt to improve accessibility in recharts. According to Web Content Accessibility Guideline there should not be loss of content when letter-spacing is increased upto 0.12em. CartesianAxis component wasn't taking into consideration letter-spacing while calculating size of ticks. I am trying to fix it. (NOTE: All tests pass)
LineChart before fixing (with letter-spacing 0.5em):
Screen Shot 2022-07-10 at 9 09 22 PM
LineChart after fixing:
Screen Shot 2022-07-10 at 9 10 57 PM

@cscrosati
Copy link

This is clearly a good improvement since there is no more overlap, which is stated in the WCAG Understanding document as a failure.

Copy link

@cscrosati cscrosati left a comment

Choose a reason for hiding this comment

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

Can you add tests to the PR to ensure the functionality doesn't regress?

@saghan
Copy link
Contributor Author

saghan commented Jul 17, 2022

Can you add tests to the PR to ensure the functionality doesn't regress?

Done.

@cscrosati
Copy link

I have another test suggestion:

FontSize/TextSpacing is X1/Y1, and the ticks are {a/b/c}
then when changing FontSize/TextSpacing to X2/Y2, ticks are now {a/c}

Something that confirms that applying a new computed style values does indeed discard or add the ticks as needed. Also independently change FontSize and TextSpacing to prove they independently are impacting the result.

@saghan
Copy link
Contributor Author

saghan commented Jul 19, 2022

I have another test suggestion:

FontSize/TextSpacing is X1/Y1, and the ticks are {a/b/c} then when changing FontSize/TextSpacing to X2/Y2, ticks are now {a/c}

Something that confirms that applying a new computed style values does indeed discard or add the ticks as needed. Also independently change FontSize and TextSpacing to prove they independently are impacting the result.

getComputedStyle does not work as expected with Karma. That's why the change works fine in a sample but not in test browser. So I have added a test to ensure that at least the states are picked correctly when getComputedStyle is stubbed.

@cscrosati
Copy link

@arcthur @xile611 This change was made to improve the accessibility of the axis overlapping ticks. This resolves concern with https://www.w3.org/WAI/WCAG22/Understanding/text-spacing.html requirements. Let us know if you have additional concerns with this proposed solution.

@cscrosati
Copy link

Note: this PR provides is supporting #2801

@jdom
Copy link

jdom commented Aug 17, 2022

LGTM. Thanks for contributing this. We are running into some of these accessibility issues in our planning phase on whether to use the library for our project too.

@arcthur arcthur merged commit 6be367c into recharts:master Aug 18, 2022
arcthur added a commit that referenced this pull request Sep 6, 2022
* 'master' of https://github.com/recharts/recharts:
  Add inactiveShape prop to Pie component (#2900)
  Revert "chore: move type deps into devDependencies (#2843)" (#2942)
  Fix typing of default tooltip formatter (#2924)
  done (#2936)
  Take letter-spacing and font-size into consideration while rendering ticks (#2898)
  Add formatter function type to tooltip props (#2916)
  doc: Update CHANGELOG.md about d3 7.x (#2919)
@nmoinvaz
Copy link

nmoinvaz commented Dec 1, 2022

It seems the calls to getElementsByClassName introduced in this PR broke IE 11 support. I didn't see any babel polyfill that would provide this function and the one polyfill that I did try didn't work. https://gist.github.com/eikes/2299607

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.

None yet

5 participants