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

Versatile clipping #6642

Merged
merged 5 commits into from
Nov 10, 2019
Merged

Versatile clipping #6642

merged 5 commits into from
Nov 10, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Oct 29, 2019

This PR changes the default clipping so it will clip at chartArea edge, if the scale defining that edge is limited at that end (by ticks.min/ticks.max). If min/max is not defined, the current clipping rules apply (borderWidth / 2 outside chartArea)

Overriding this clipping is also allowed per dataset:

clip: 10 // would clip 10px outside chartArea on each edge
clip: -10 // would clip 10px inside chartArea on each edge
clip: false // would not clip
clip: 0 // would clip to chartArea
clip: { left: 10, right: -10, top: false, bottom: 0) // per edge config

Fixes: #6631

TODO

  • documentation
  • clipping in core, enabling for any dataset type

Thoughts?

@benmccann
Copy link
Contributor

Does this draw the image from #6631 (comment) ?

@kurkle
Copy link
Member Author

kurkle commented Oct 29, 2019

Yes, by default.

benmccann
benmccann previously approved these changes Oct 29, 2019
@kurkle
Copy link
Member Author

kurkle commented Oct 30, 2019

Updated the description.

@kurkle kurkle requested a review from etimberg October 31, 2019 07:18
etimberg
etimberg previously approved these changes Oct 31, 2019
@kurkle kurkle changed the title Versatile clipping for lines Versatile clipping Oct 31, 2019
@kurkle kurkle dismissed stale reviews from etimberg and benmccann via cf2e905 November 5, 2019 18:45
@benmccann
Copy link
Contributor

@kurkle I notice this is still in draft mode. Are you ready for us to review?

@kurkle kurkle marked this pull request as ready for review November 5, 2019 20:16
@kurkle
Copy link
Member Author

kurkle commented Nov 5, 2019

Yeah, was waiting on CC/CI and got carried away in the mean time :)

src/controllers/controller.bubble.js Outdated Show resolved Hide resolved
@@ -769,8 +773,17 @@ helpers.extend(Chart.prototype, /** @lends Chart */ {
return;
}

helpers.canvas.clipArea(ctx, {
left: clip.left === false ? 0 : area.left - clip.left,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprise the default is false instead of null or undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Default is false only for controllers not overriding getMaxOverflow
false meaning no clipping. Because the clipping could be off on one edge only, false is tested and clipping area extended to that edge of canvas.

src/elements/element.point.js Show resolved Hide resolved
@benmccann
Copy link
Contributor

@kurkle looks like this one will need to be rebased

@kurkle
Copy link
Member Author

kurkle commented Nov 10, 2019

Updated the fixtures to reflect the removal of zeroLine* functionality.
The grids are intentionally visible in these, to make the tests easily understandable.

Just chartArea borders would suffice, but I don't think thats currently (easily) doable.

@etimberg etimberg merged commit 11ef1e5 into chartjs:master Nov 10, 2019
@etimberg etimberg added this to the Version 3.0 milestone Nov 10, 2019
@kurkle kurkle deleted the line-clip branch November 13, 2019 06:10
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.

Line drawn outside min/max since 2.9
3 participants