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

[Bugfix] Remedy for Dual Axis chart render bug #4130

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Mogball
Copy link
Contributor

@Mogball Mogball commented Dec 28, 2017

An attempt to remedy #4115, which seems to be the result of issues handling annotations for Dual Axis Line Charts.

The first issue is that the most recent nvd3 release 1.8.6 does not expose xScale (although it exists in nvd3/master), and secondly there is yAxis1 and yAxis2 instead of just yAxis.

Making up for xScale isn't an issue because multiChart just uses d3.scale.linear() but I'm wondering how annotations are handled for two axes? @fabianmenges

@kkalyan
Copy link
Contributor

kkalyan commented Dec 28, 2017

thanks @Mogball, this fixed #4115.

@fabianmenges
Copy link
Contributor

fabianmenges commented Dec 28, 2017

Right now, annotations are not supported with the Dual Axis Line Chart slices (in the sense that I have not tested and developed with them in mind and I'm pretty sure they are not available through the UI).

@Mogball Your fix looks good, but there are probably some other things that need to be hooked up, for it to actually work e.g.: https://github.com/apache/incubator-superset/blob/master/superset/assets/javascripts/modules/AnnotationTypes.js#L59

@kkalyan did you setup annotations in a time series 'single'-line chart and then switch over to dual line chart? We probably need to add some code that wipes/ignores annotations if the current slice/chart type does not support the type of annotation configured.

@Mogball
Copy link
Contributor Author

Mogball commented Dec 28, 2017

I seemed to have been getting the same errors even though I didn't create any annotation layers.

@mistercrunch
Copy link
Member

Lacking the whole context, should I merge this?

Copy link
Contributor

@fabianmenges fabianmenges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug, we should merge it.

@fabianmenges
Copy link
Contributor

@Mogball you are right, this is an issue even when no annotations are selected.

@mistercrunch mistercrunch merged commit f7c5527 into apache:master Jan 2, 2018
@Mogball Mogball deleted the jeff/dual_axis branch January 3, 2018 01:51
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants