rrag / react-stockcharts

Highly customizable stock charts with ReactJS and d3
http://rrag.github.io/react-stockcharts
MIT License
3.89k stars 961 forks source link

Chart disappears on zoom #215

Closed stevesouth closed 7 years ago

stevesouth commented 7 years ago

branch: next

One mobile pinch zooming in and out causes the whole canvas to disappear every now and again. Touching and panning then seems to bring it back.

rrag commented 7 years ago

there is some weird behavior while touching with a second finger while doing a one finger pan, the chart appears to be stuck. But I am not able to reproduce the pinch zoom in, out you have explained. Can you try again, not sure if it was fixed differently or I am just not seeing it.

the documentation build with next branch is - http://rrag.github.io/react-stockcharts-next/documentation.html#/load_more_data_on_pan

stevesouth commented 7 years ago

I am just looking at the simple /lib/charts/CandleStickChart. It seems to be far more difficult to make it happen in the latest next branch, but its still possible. I am trying to work out why, but there is no error being thrown.

stevesouth commented 7 years ago

This might be a red herring, but I am seeing lots of errors in ChartDataUtil on line 141

? chartsToPan.indexOf(id) > -1

chartsToPan is a syntetic touch event, and therefore has no indexOf. This is when zooming / panning on mobile.

If I avoid this error by checking for .indexOf I haven't yet been able to reproduce the chart disappearing, but as I say I am not 100% sure these two issues are related.

rrag commented 7 years ago

Thats great, thanks for troubleshooting this. may I ask how you were able to identify this? I dont have a mac, and use linux (Manjaro) for all my dev, any pointers so I can look at console logs from an ipad?

stevesouth commented 7 years ago

The chartsToPan.indexOf issue you can see in chrome dev tools (chrome://inspect) when connecting an Android phone, it happens on most pans.

The chart disappearing issue I can now reproduce reliably.

1) With a single finger, pan back and forth a few times and don't release 2) Place a second finger on screen and zoom 3) The chart disappears

stevesouth commented 7 years ago

Looking at it again I think the indexOf error is related, but not the only cause. If I guard against it the chart disappearing issue is far harder to reproduce.

stevesouth commented 7 years ago

EventCapture.jsx line 321 has

onPanEnd(newPos, panStartXScale, panOrigin, e);

Every other call to onPanEnd in that file looks like this (includes chartsToPan)

onPanEnd(newPos, panStartXScale, panOrigin, chartsToPan, e);

When called without chartsToPan, eventually you end up in ChartDataUtil.getChartConfigWithUpdatedYScales, with a chartsToPan which is set to the event (e), so it is defined, but has no indexOf.

I am not sure what the correct fix is as I am not familiar enough with the codebase yet, but the options are:

1) Pass in chartsToPan when calling onPanEnd (i'm not sure if that makes sense) 2) or onPanEnd(newPos, panStartXScale, panOrigin, null, e);

rrag commented 7 years ago

thanks a ton, I learned a lot about debugging on mobile today. \o/

rrag commented 7 years ago

fixed in next and documentation site also pushed up. check it out