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

Series.cropData throws error for wind barb series #8713

Closed
luuuke opened this Issue Jul 31, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@luuuke

luuuke commented Jul 31, 2018

Expected behaviour

Wind barb's data gets cropped and later displayed.

Actual behaviour

cropData throws an error as yData is undefined for the wind barb series type.
Catching the case of yData being undefined and returning an undefined value for yData makes everything work again.

Live demo with steps to reproduce

http://jsfiddle.net/4saL58wu/2/

Notice how the xAxis' min is later than the series' pointStart. Also cropThreshold is set to 0 for the series to always crop its data. If setting the min to the same value as pointStart or cropThreshold to a higher value, the error does not occur, as cropData is not called.

Product version

Highcharts, Highstock v6.1.1

@luuuke

This comment has been minimized.

Show comment
Hide comment
@luuuke

luuuke Jul 31, 2018

For now I "fixed" it by wrapping cropData and passing a bogus yData to it:

Highcharts.wrap(Highcharts.Series.prototype, 'cropData', function (proceed, xData, yData, min, max, cropShoulder) {
    if (typeof yData === 'undefined') {
        yData = {
            slice: _.noop
        };
        let croppedData = proceed.call(this, xData, yData, min, max, cropShoulder);
        croppedData.yData = undefined;
        return croppedData;
    } else {
        return proceed.call(this, xData, yData, min, max, cropShoulder);
    }
});

luuuke commented Jul 31, 2018

For now I "fixed" it by wrapping cropData and passing a bogus yData to it:

Highcharts.wrap(Highcharts.Series.prototype, 'cropData', function (proceed, xData, yData, min, max, cropShoulder) {
    if (typeof yData === 'undefined') {
        yData = {
            slice: _.noop
        };
        let croppedData = proceed.call(this, xData, yData, min, max, cropShoulder);
        croppedData.yData = undefined;
        return croppedData;
    } else {
        return proceed.call(this, xData, yData, min, max, cropShoulder);
    }
});
@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Aug 1, 2018

Contributor

Hi @luuuke

Thank you for reporting the issue and for the workaround! It looks like a bug in our docs, cropThreshold is not supported by windbarb series:

* @excluding boostThreshold,marker,connectEnds,connectNulls,cropThreshold,
* dashStyle,gapSize,gapUnit,dataGrouping,linecap,shadow,stacking,
* step

Of course, this should be fixed, we can exceed default 200 points and this will be broken again: http://jsfiddle.net/BlackLabel/4saL58wu/8/

Internal note:
It looks like all options from this exclude exist in the docs, we should remove this. The same applies to vector or pareto series, but sankey is fine. The only difference I see:

Contributor

pawelfus commented Aug 1, 2018

Hi @luuuke

Thank you for reporting the issue and for the workaround! It looks like a bug in our docs, cropThreshold is not supported by windbarb series:

* @excluding boostThreshold,marker,connectEnds,connectNulls,cropThreshold,
* dashStyle,gapSize,gapUnit,dataGrouping,linecap,shadow,stacking,
* step

Of course, this should be fixed, we can exceed default 200 points and this will be broken again: http://jsfiddle.net/BlackLabel/4saL58wu/8/

Internal note:
It looks like all options from this exclude exist in the docs, we should remove this. The same applies to vector or pareto series, but sankey is fine. The only difference I see:

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Aug 20, 2018

Collaborator

@sophiebremer Will you have a look at why the syntax matters? Propably the easiest fix is just to make sure we use the one syntax that works.

Collaborator

TorsteinHonsi commented Aug 20, 2018

@sophiebremer Will you have a look at why the syntax matters? Propably the easiest fix is just to make sure we use the one syntax that works.

@luuuke

This comment has been minimized.

Show comment
Hide comment
@luuuke

luuuke Aug 22, 2018

I'm not sure if I'm understanding you right. So the guess is, that setting cropThreshold explicitly in the series config is causing it and by adjusting the docs it will be fixed?
In my observations the error also occurs when not supplying cropThreshold (see http://jsfiddle.net/4saL58wu/13/). As a default value is then used instead, just more data is required to cause this problem.

luuuke commented Aug 22, 2018

I'm not sure if I'm understanding you right. So the guess is, that setting cropThreshold explicitly in the series config is causing it and by adjusting the docs it will be fixed?
In my observations the error also occurs when not supplying cropThreshold (see http://jsfiddle.net/4saL58wu/13/). As a default value is then used instead, just more data is required to cause this problem.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Aug 22, 2018

Contributor

Hi @luuuke

This will be fixed in the core too. Simply we have two issues here: wrong docs and bug in code. As I said above:

Of course, this should be fixed, we can exceed default 200 points and this will be broken again: http://jsfiddle.net/BlackLabel/4saL58wu/8/

Contributor

pawelfus commented Aug 22, 2018

Hi @luuuke

This will be fixed in the core too. Simply we have two issues here: wrong docs and bug in code. As I said above:

Of course, this should be fixed, we can exceed default 200 points and this will be broken again: http://jsfiddle.net/BlackLabel/4saL58wu/8/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment