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

Apply lineJoin style at the first point in radar charts #6269

Merged
merged 2 commits into from
May 20, 2019

Conversation

nagix
Copy link
Contributor

@nagix nagix commented May 12, 2019

lineJoin style is not applied to the first point in radar charts. This PR fixes it by adding ctx.closePath if the line has a close path. If it is open, it shifts elements in the point array so that the line is correctly joined at the first point.

I will add image tests in #6263 if this PR is merged first.

Master: https://jsfiddle.net/nagix/bf2geq3o/
Screen Shot 2019-05-12 at 1 51 02 PM

This PR: https://jsfiddle.net/nagix/e36dp7k8/
Screen Shot 2019-05-12 at 1 51 14 PM

@kurkle
Copy link
Member

kurkle commented May 12, 2019

Just a random note: With data: [1, 1, null, 1, 1], miter gets cropped by chart area.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

IMO that mock canvas test should be removed.

@nagix
Copy link
Contributor Author

nagix commented May 13, 2019

@kurkle The radar chart create a gap with NaN but not with null. This is not consistent with the line chart, so we may need to fix if it's a bug.

@nagix
Copy link
Contributor Author

nagix commented May 13, 2019

I agree to remove mock tests if we add image tests.

@nagix
Copy link
Contributor Author

nagix commented May 14, 2019

Null value resetting to minimum was introduced in #1436, but I'm not sure what this is for. @etimberg Do you have any comment on this?

@nagix
Copy link
Contributor Author

nagix commented May 15, 2019

@kurkle I may have misunderstood your comment - I think the cropped line border with miter is not an issue because it could happen in other chart types and it can be controlled using layout.padding.

On the other hand, null handing is still in question.

@kurkle
Copy link
Member

kurkle commented May 15, 2019

null handling can be done separately if needed.

@etimberg
Copy link
Member

@nagix not sure why the null handling was added there. I think it should be skipped like other charts

@nagix
Copy link
Contributor Author

nagix commented May 16, 2019

Removed loop line mock tests.

@nagix
Copy link
Contributor Author

nagix commented May 17, 2019

#6282 was created to fix the null handling.

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
Development

Successfully merging this pull request may close these issues.

5 participants