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

Allow configuration of borderWidth as object #6077

Merged
merged 13 commits into from
Feb 25, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Feb 18, 2019

Alternative to #6047

Pen (ef842ad)
Pen (c5df9c9)

Fixes: #5565
Fixes: #5071
Fixes: #5709
Fixes: #4681
Related: #3293 (fixed in master already)

@kurkle

This comment has been minimized.

@kurkle
Copy link
Member Author

kurkle commented Feb 18, 2019

both, me and codeclimate are happy about this now, feel free to review 😄

nagix
nagix previously approved these changes Feb 19, 2019
Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

I played with the pen for a while, and it all look good to me. Reducing the number of lines in element.rectangle.js seems to be a good sign😊

etimberg
etimberg previously approved these changes Feb 19, 2019
@kurkle kurkle dismissed stale reviews from etimberg and nagix via 4cc6628 February 20, 2019 13:38
@kurkle
Copy link
Member Author

kurkle commented Feb 20, 2019

After a long chat with @simonbrunel and many tests with jsperf, applied some optiomizations to parsing. Also added a regression test for chart area clipping.

Added regression test was inspired by the thought of it not working after this PR. That turns out to be false.

MDN gives a misleading description to clip:

The CanvasRenderingContext2D.clip() method of the Canvas 2D API turns the current or given path into the current clipping region. It replaces any previous clipping region.

HTML Standard:

The clip() method, when invoked, must create a new clipping region by calculating the intersection of the current clipping region and the area described by the intended path, using the fill rule indicated by the fillRule argument. Open subpaths must be implicitly closed when computing the clipping region, without affecting the actual subpaths. The new clipping region replaces the current clipping region.

benmccann
benmccann previously approved these changes Feb 20, 2019
@kurkle
Copy link
Member Author

kurkle commented Feb 20, 2019

After loads of more talk and testing with @simonbrunel, decided to go with option C

image

A
This is how master handles semi-transparent borders.

B
Previous version of this PR.

C
Whole bar is filled with background color and borders are drawn on top of that.
This removes remaining glitches between border and background. It also simplifies the drawing even more!
Note: These glitches appear only when devicePixelRatio != 1
Note2: I had to use still clip to remove faint outer borders on Chrome when on the edges there should be no border (the original fill problem @nagix)

@kurkle
Copy link
Member Author

kurkle commented Feb 20, 2019

Visualizing the difference:
borders

@simonbrunel simonbrunel requested a review from nagix February 21, 2019 16:32
Copy link
Contributor

@nagix nagix left a comment

Choose a reason for hiding this comment

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

Option C looks good. The result is consistent with the arc element with borderAlign: 'inner'.

@simonbrunel simonbrunel merged commit 0ec3f55 into chartjs:master Feb 25, 2019
@simonbrunel
Copy link
Member

Thanks @kurkle for this great work!

@kurkle kurkle deleted the rectangle-borders2 branch May 5, 2019 06:37
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants