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

Prevent bezier points from being capped when a data point is off the chart #5937

Merged
merged 3 commits into from
Jan 5, 2019

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Dec 24, 2018

#5265 reported that horizontal lines appear in a chart. When I tried to skip the clipping for lines, I noticed that lines drawn off the chart have bezier control points on the y axis, and some lines overflow into the chart area.

screen shot 2018-12-24 at 1 03 35 pm

This PR fixes this issue by preventing bezier points from being capped when a data point is off the chart.

Version 2.7.3: https://jsfiddle.net/nagix/e40qgfzs/

screen shot 2018-12-24 at 1 36 33 pm

Master + This PR: https://jsfiddle.net/nagix/h8dp4ft6/

screen shot 2018-12-24 at 1 36 51 pm

Fixes #5265

var area = me.chart.chartArea;
var points = (meta.data || []);
var lineModel = meta.dataset._model;
var area = chart.chartArea;
Copy link
Contributor

Choose a reason for hiding this comment

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

I might call this chartArea to be a little clearer. And I think model would probably be fine for the previous variable. But I don't feel strongly about either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

model is used to keep point._model later, so this needs to be a different name.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer area, which I think is clear enough in that context. I also prefer model but as you said it's already taken.

benmccann
benmccann previously approved these changes Jan 2, 2019
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

This looks good to me though someone more familiar with the bezier stuff should probably take a look as well

etimberg
etimberg previously approved these changes Jan 3, 2019
@etimberg etimberg requested a review from simonbrunel January 3, 2019 01:32
src/helpers/helpers.canvas.js Outdated Show resolved Hide resolved
var area = me.chart.chartArea;
var points = (meta.data || []);
var lineModel = meta.dataset._model;
var area = chart.chartArea;
Copy link
Member

Choose a reason for hiding this comment

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

I prefer area, which I think is clear enough in that context. I also prefer model but as you said it's already taken.

src/controllers/controller.line.js Outdated Show resolved Hide resolved
@nagix nagix dismissed stale reviews from etimberg and benmccann via 1ed3f50 January 4, 2019 19:21
@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 4, 2019
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @nagix

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

Successfully merging this pull request may close these issues.

Bug On Safari
5 participants