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

Axis Prop Updates #1835

Merged
merged 14 commits into from
May 12, 2021
Merged

Axis Prop Updates #1835

merged 14 commits into from
May 12, 2021

Conversation

jhumbug
Copy link
Contributor

@jhumbug jhumbug commented May 6, 2021

This provides a fix for when the domain of a dataset using VictoryZoom changes, such as switching out data on demand.

@jhumbug jhumbug requested a review from boygirl May 6, 2021 14:43
@jhumbug
Copy link
Contributor Author

jhumbug commented May 6, 2021

@boygirl Seems like this was a very simple fix. Suspiciously simple. But I couldn't find any unintended consequences of fixing it like this. Any insights on your end?

@jhumbug
Copy link
Contributor Author

jhumbug commented May 6, 2021

I looked into what you mentioned about offsetX and offsetY on Chart and Axis, but that didn't seem to be related as far as I could tell.

@boygirl
Copy link
Contributor

boygirl commented May 6, 2021

@jhumbug apologies! I think this does fix a portion of the issue, but I should have been clearer about the axis offset issue. Here's a gif that I think illustrates the problem:

axis-offset-issue

In the chart above, there are not user provided xOffset / yOffset props. The offsets are being calculated by VictoryChart based on the domain. At the beginning, the offsets are calculated correctly so that the axes intersect at (0, 0), However, when zooming, these offsets aren't changing, because VictoryZoomContainer alters the domain prop for all the children of VictoryChart, but not for VictoryChart itself.

@jhumbug
Copy link
Contributor Author

jhumbug commented May 6, 2021

Ok, yeh I missed that. I will look into that part of it now, thank you for the gif.

@jhumbug jhumbug force-pushed the fix/zoom-axis-update branch from 541f4e2 to 10ca857 Compare May 7, 2021 13:59
@boygirl
Copy link
Contributor

boygirl commented May 11, 2021

From looking at the stories, it looks like there's a systematic problem with horizontal charts and with how the range / padding is being calculated

@jhumbug
Copy link
Contributor Author

jhumbug commented May 11, 2021

From looking at the stories, it looks like there's a systematic problem with horizontal charts and with how the range / padding is being calculated

Yep, addressing those now. Most are the horizontal issue which is easy. I'll look at range/padding after.

@jhumbug
Copy link
Contributor Author

jhumbug commented May 11, 2021

Got most of the chromatic issues with those fixes. I'll work on the few left tomorrow.

@jhumbug
Copy link
Contributor Author

jhumbug commented May 12, 2021

The last chromatic changes are from added/changed tests in previous PRs and 2 (VictoryScatter: Theme & VictoryLine: Theme) that actually look correct now, rather than the previous version. They have slight differences in the grid position, but it looks like the new versions are right now, no?

@boygirl
Copy link
Contributor

boygirl commented May 12, 2021

@jhumbug A couple minor comments, but this is otherwise looking ready. And axes look so much nicer when zooming

better-zooming

@jhumbug jhumbug marked this pull request as ready for review May 12, 2021 20:18
@jhumbug jhumbug linked an issue May 12, 2021 that may be closed by this pull request
4 tasks
@jhumbug jhumbug changed the title Fix - Zoom Domain Update Axis Prop Updates May 12, 2021
@boygirl boygirl merged commit 36c587e into main May 12, 2021
@boygirl boygirl deleted the fix/zoom-axis-update branch May 12, 2021 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants