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

Funnel and broke-axis compatibility #5950

Closed
pawelfus opened this Issue Nov 9, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@pawelfus
Contributor

pawelfus commented Nov 9, 2016

Actual behaviour

We get an error in js console when loading "broken-axis" before "funnel". Changing order resolves the issue, but this is a slippery slope. We either need to resolve order of wrap() methods or write a guide with correct order for modules. Guide may not be enough, I can imagine issue like: module A before B, module C before A, but (!) module module B before module C.

Live demo with steps to reproduce

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Nov 10, 2016

Collaborator

Do you know what specific wrap call causes it? In some cases it may be better to refactor it or add event handlers.

Collaborator

TorsteinHonsi commented Nov 10, 2016

Do you know what specific wrap call causes it? In some cases it may be better to refactor it or add event handlers.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Nov 10, 2016

Contributor

This wrap: wrap(H.seriesTypes.column.prototype, 'drawPoints', drawPointsWrapped); but after debugging, it's caused by using column drawPoints method in funnel: drawPoints: seriesTypes.column.prototype.drawPoints,.

broken-axis module assumes that column series has axes (which is correct), but funnel series doesn't have them, it only reuses column's prototype method.

PS: I don't have a real use-case for it, issue arrived when testing Highcharts-editor.

Contributor

pawelfus commented Nov 10, 2016

This wrap: wrap(H.seriesTypes.column.prototype, 'drawPoints', drawPointsWrapped); but after debugging, it's caused by using column drawPoints method in funnel: drawPoints: seriesTypes.column.prototype.drawPoints,.

broken-axis module assumes that column series has axes (which is correct), but funnel series doesn't have them, it only reuses column's prototype method.

PS: I don't have a real use-case for it, issue arrived when testing Highcharts-editor.

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Nov 10, 2016

Collaborator

Okay. Probably the broken-axis plugin should actually check for axis...

Collaborator

TorsteinHonsi commented Nov 10, 2016

Okay. Probably the broken-axis plugin should actually check for axis...

@havgry

This comment has been minimized.

Show comment
Hide comment
@havgry

havgry Nov 22, 2016

I do see the reference to #6004 but are you guys aware that this bug breaks the funnel chart simply by using Highstock.js? It's effectively preventing us from upgrading to v5.

havgry commented Nov 22, 2016

I do see the reference to #6004 but are you guys aware that this bug breaks the funnel chart simply by using Highstock.js? It's effectively preventing us from upgrading to v5.

@pawelfus pawelfus added the Regression label Nov 22, 2016

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Nov 22, 2016

Contributor

You are right, it used to work in 4.x versions: http://jsfiddle.net/dko6016u/4/

Contributor

pawelfus commented Nov 22, 2016

You are right, it used to work in 4.x versions: http://jsfiddle.net/dko6016u/4/

@havgry

This comment has been minimized.

Show comment
Hide comment
@havgry

havgry Nov 30, 2016

Any news on this? It seems rather odd that this issue is not being addressed at all.

havgry commented Nov 30, 2016

Any news on this? It seems rather odd that this issue is not being addressed at all.

@pawelfus pawelfus self-assigned this Nov 30, 2016

@pawelfus pawelfus added this to the 5.0.6 milestone Nov 30, 2016

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Nov 30, 2016

Contributor

Internal note:
After all we need to check this. We can go back to 4.x solution, where drawPoints was not inherited from column series, as an alternative, but the above makes more sense.

Contributor

pawelfus commented Nov 30, 2016

Internal note:
After all we need to check this. We can go back to 4.x solution, where drawPoints was not inherited from column series, as an alternative, but the above makes more sense.

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