accessibilityLayer should respect a reversed XAxis #4225
Conversation
|
@ckifer I can't remember, should this target |
|
It could exist in both, if we want to backport it to 2.x as a bugfix I can release it in 2.12.2, I have something else to release as well |
There was a problem hiding this comment.
Thanks for the changes, LGTM.
@PavelVanecek from an API perspective do you think reversed should exist on the base axis? Imo Z axis is a totally different category and shouldn't really share any props, but since they already do... Better to be on the individual components maybe?
|
I don't understand the purpose of reversed axis, any of them. If an axis is reversed then the whole chart should be reversed, no? So then I would do |
|
It's for RTL languages (Arabic, hebrew, etc). The charts origin would be in the bottom right, rather than the bottom left. |
|
Yeah +1. Can't just change your data order if we always assume you read left to right. @julianna-langston I think we should move reversed to exist only on the XAxis. We don't need to support RTL vertically correct? |
|
So, Since that change is going to leak into the ZAxus, I'll see if I can find another way to wrangle the types. |
|
Lol oops my bad, I was thinking it was new. Maybe we could type narrow somehow to the specific axis types. Imo all good to put TS-expect-error there for now so we know to fix it later |
|
If it's for RTL reading should this be limited to xAxis only? Should we also revert Legend? Also, reverse implies that there's one correct direction and one reversed direction. Which one is correct one depends on culture. So should we then have a direction=ltr|rtl as a whole chart prop? |
|
It was already a thing in YAxis before any of us, without thinking of RTL, etc. you were able to reverse both Axes. If we had a chart prop then yeah it should only affect XAxis. Chart prop could take precedence over reversed. As for Legend, we could ask the issue requester, he would know better than us. But it's easier to make a custom legend than a custom axis |
| @@ -1099,6 +1100,8 @@ export const generateCategoricalChart = ({ | |||
| coordinateList: this.state.tooltipTicks, | |||
| mouseHandlerCallback: this.triggeredAfterMouseMove, | |||
| layout: this.props.layout, | |||
| // Assuming there's only 1 <XAxis />, check to see if it has reversed={true}. If so, this is an RTL chart | |||
There was a problem hiding this comment.
What happens if there are multiple axes?
| @@ -172,6 +195,83 @@ describe('AccessibilityLayer', () => { | |||
| expect(mockMouseMovements.mock.instances).toHaveLength(0); | |||
| }); | |||
|
|
|||
| test('Left/right arrow pays attention to if the XAxis is reversed', () => { | |||
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.x #4225 +/- ##
=======================================
Coverage 93.25% 93.26%
=======================================
Files 95 95
Lines 20130 20159 +29
Branches 2736 2746 +10
=======================================
+ Hits 18772 18801 +29
Misses 1347 1347
Partials 11 11 ☔ View full report in Codecov by Sentry. |
|
@julianna-langston just merging 3.x into your branch so that coverage generates correctly |
Description
The AccessibilityManager (which manages the behavior for the
accessibilityLayerprop) will take into account if there is an<XAxis reversed={true} />child. If so, the left/right arrow keys will reverse, so that they align with the chart's visuals.Related Issue
#4214
Motivation and Context
How Has This Been Tested?
Tested manually, and added test cases
Screenshots (if appropriate):
Types of changes
Checklist: