Skip to content

Conversation

@cholt0425
Copy link

Added ability to draw a point as an inverted triangle as it is useful
for showing progress along a bar, line, or scale.

Added ability to draw a point as an inverted triangle as it is useful
for showing progress along a bar, line, or scale.
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.

Code looks good. Thanks for updating the documentation. I think it would be worthwhile to add a test for this to https://github.com/chartjs/Chart.js/blob/master/test/specs/element.point.tests.js

@simonbrunel
Copy link
Member

simonbrunel commented Dec 28, 2017

At this point, it would be much better to expose a new option (maybe pointRotation) instead of duplicating the drawing code for every use case because we could also have triangleRot (45°), triangleRotInv (-45°), triangleLeft (90°), triangleRight (-90°), etc. Then, we could deprecate crossRot (eq. cross + 45°) and RectRot (eq. rect + 45°), and it would also benefit to rectRounded, line and star without duplicating the drawing logic.

@cholt0425
Copy link
Author

I'll add some tests. Question, I'm a newbie to using GitHub. Is there a way to add the tests and update this pull request or do you just submit an entirely new one? I just want to be sure that I make the change properly.

@cholt0425
Copy link
Author

@simonbrunel I completely agree that would be a great feature. However, that is a huge breaking change for backwards compatibility. Perhaps in a major release chartjs can support both methods for a time: "Legacy Names" and "Name and Rotation"

Added a unit test to test out that having a point with style
"triangleInv" draws and inverted triangle.
@simonbrunel
Copy link
Member

To update that PR, simply push new commits to your "master" branch.

There is no breaking change, we still use pattern names and add support for rotation (which will be handled by a new option). We deprecate *Rot patterns (basically, remove them for the docs). There should be only one additional canvas rotation and since the default for this new option will be 0, old code still work as expected.

@cholt0425
Copy link
Author

@simonbrunel Thanks a bunch! It appears that worked nicely! I wanted to make sure I didn't disrupt things if I did it incorrectly. I submitted the unit test. The code climate checks wants me to refactor the beginning portion of the calculation but I'm thinking I should just not do that as it would probably add more complication for not much benefit. Do you agree?

@simonbrunel
Copy link
Member

What I mean is that I would not add this extra string (triangleInv) since it duplicates code and calls for even more patterns, but instead introduce a new option to control the pattern rotation. Thus I would not merge that PR as is.

(though I agree to ignore CodeClimate report :) )

@benmccann
Copy link
Contributor

@cholt0425 do you plan to update this PR in the way that Simon suggested? I'm wondering if we should leave this open or close it...

@benmccann
Copy link
Contributor

@cholt0425 I would really love to see this feature and would love to help review any PR ad get it merged. I'm going to close this PR since it's been inactive for awhile, but please consider reopening or submitting a new PR to support the pointRotation option!

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.

4 participants