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

Implement clipping #3658

Merged
merged 2 commits into from
Dec 3, 2016
Merged

Implement clipping #3658

merged 2 commits into from
Dec 3, 2016

Conversation

KoyoSE
Copy link
Contributor

@KoyoSE KoyoSE commented Nov 30, 2016

I tried implementing clipping chart area.
I would be pleased if you like it.

Resolves #3506, #3491, #2873

Bar and Line chart.

Before

non clip bar line

https://jsfiddle.net/KoyoSE/n7ben0fx/

After

clip bar line

https://jsfiddle.net/KoyoSE/98x7ny4b/
(Note: jsfeddle takes time to execute.This is because I put the built chart.js in the html area.)
Line chart: The point fades out when going out.

Radar and Scatter chart.

Before

non clip radar scatter

https://jsfiddle.net/KoyoSE/fwt4bxcc/
(Do Radar chart have to consider the drawing of the points below min?)

After

clip radar scatter

https://jsfiddle.net/KoyoSE/7v3ehcqt/
(Note: jsfeddle takes time to execute.This is because I put the built chart.js in the html area.)
Radar chart: It works the same before. Clipping is not done.
The Radar chart, the following error is displayed on the console.
'Unable to get property 'xAxes' of undefined or null reference'
This is solved by removing #3620.

Other

  1. It may be better to display the tool tip position in the chart area.
  2. Drawing Bezier curve is slightly strange. Especially outside the chart area.

chart.js version v2.4.0

@@ -286,20 +286,23 @@ module.exports = function(Chart) {
var points = meta.data || [];
var easingDecimal = ease || 1;
var i, ilen;
var chartArea = me.chart.chartArea;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I made a mistake a bit. this is no need.


// Draw the points
for (i=0, ilen=points.length; i<ilen; ++i) {
points[i].draw();
points[i].draw(chartArea);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

points[i].draw(me.chart.chartArea);

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

I agree that we should put the tooltip in the chart area.

var errMarginPercent = 1.01; // 1.01 is margin for Accumulated error. (Especially Edge, IE.)
var ratio = 0;
// going out from inner charArea?
if ((chartArea !== undefined)&&(isOutofChartArea(this._view, chartArea, errMarginPercent))&&(isOutofChartArea(this._model, chartArea, errMarginPercent))) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you should only need to check this._model here. At the end of the animation this._view will have the same values as this._model

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you are right. Then no need var isOutofChartArea = function(... and the code will be shorter. Thank you.

// going out from inner charArea?
if ((chartArea !== undefined)&&(isOutofChartArea(this._view, chartArea, errMarginPercent))&&(isOutofChartArea(this._model, chartArea, errMarginPercent))) {
// Point fade out
if (this._model.x < chartArea.left) {
Copy link
Member

Choose a reason for hiding this comment

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

If you capture this in a local variable, this will have better minification.

var me = this;
if (me._model.x < chartArea.left) {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for checking.
I will change. 😄

@KoyoSE
Copy link
Contributor Author

KoyoSE commented Dec 1, 2016

I changed the code.
Thank you for your review.

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

LGTM

CC @chartjs/maintainers

@KoyoSE
Copy link
Contributor Author

KoyoSE commented Dec 2, 2016

I updated jsfiddle with my new code.

I tested it. I think there is OK.

After

Bar and Line

https://jsfiddle.net/KoyoSE/98x7ny4b/8/

Radar and Scatter

https://jsfiddle.net/KoyoSE/7v3ehcqt/2/

Other

It is building with chart.js of the current(02/Dec/2016) source code.
Tooltip labels are OK. and no more error on Radar chart. (reference #3655)

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.

4 participants