Skip to content

Fixes for x/yValue for inverted charts#66

Merged
pawelfus merged 2 commits intoblacklabel:masterfrom
eltonjude:fix/bar-chart-xy-value
Oct 26, 2016
Merged

Fixes for x/yValue for inverted charts#66
pawelfus merged 2 commits intoblacklabel:masterfrom
eltonjude:fix/bar-chart-xy-value

Conversation

@eltonjude
Copy link

@eltonjude eltonjude commented Oct 24, 2016

  • Flip the computed x and y values if chart is inverted
  • Flip the axis used for computing width and height if chart is inverted and units = 'values'

In this example, I want to draw a rectangle in the middle of the first bar, starting at 25px and having a width of 50px.
Existing broken case: http://jsfiddle.net/gnb0pos7/
With the fixes in this PR: http://jsfiddle.net/gnb0pos7/1/

Works fine for column chart with and without these fixes:
Without: http://jsfiddle.net/gnb0pos7/2/
With: http://jsfiddle.net/gnb0pos7/3/

});
} else {
group.translate(x, y);
setTimeout(function() {
Copy link
Collaborator

@pawelfus pawelfus Oct 25, 2016

Choose a reason for hiding this comment

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

Hmm, what exactly does it fix? If chart.animation is disabled, then we do we need to defer group translation?

Copy link
Author

Choose a reason for hiding this comment

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

series animation can be true even if chart.animation returns false. I think what I need to do here is to do an else if and check if series animation exists and if yes, do a group.animate. I'll remove this commit from this PR and post it via a different PR.

@eltonjude eltonjude force-pushed the fix/bar-chart-xy-value branch from 41761f0 to ee9782a Compare October 25, 2016 15:10
@eltonjude
Copy link
Author

@pawelfus I have removed commit eltonjude@05e570b from this PR since it had nothing to do with the purpose of this PR in the first place.

@pawelfus pawelfus merged commit f9b233e into blacklabel:master Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants