FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Allow configuration of borderWidth as object by kurkle · Pull Request #6077 · chartjs/Chart.js · GitHub

Allow configuration of borderWidth as object - #6077

Merged
simonbrunel merged 13 commits into
chartjs:masterfrom
kurkle:rectangle-borders2
Feb 25, 2019
Merged

Allow configuration of borderWidth as object#6077
simonbrunel merged 13 commits into
chartjs:masterfrom
kurkle:rectangle-borders2

Conversation

kurkle commented Feb 18, 2019
edited
Loading

Copy link
Copy Markdown
Member

Alternative to #6047

Pen (ef842ad)
Pen (c5df9c9)

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

This comment has been minimized.

kurkle commented Feb 18, 2019

Copy link
Copy Markdown
Member Author

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

Comment thread src/elements/element.rectangle.js Outdated
nagix previously approved these changes Feb 19, 2019

nagix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 previously approved these changes Feb 19, 2019
kurkle dismissed stale reviews from etimberg and nagix via 4cc6628 February 20, 2019 13:38

kurkle commented Feb 20, 2019
edited
Loading

Copy link
Copy Markdown
Member Author

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 previously approved these changes Feb 20, 2019

kurkle commented Feb 20, 2019

Copy link
Copy Markdown
Member Author

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

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 commented Feb 20, 2019

Copy link
Copy Markdown
Member Author

Visualizing the difference:

Comment thread src/elements/element.rectangle.js Outdated
simonbrunel requested a review from nagix February 21, 2019 16:32

nagix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

simonbrunel merged commit 0ec3f55 into chartjs:master Feb 25, 2019

Copy link
Copy Markdown
Member

Thanks @kurkle for this great work!

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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

5 participants


Back | FazBrowse Home | New Git URL