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

Maintain the label always inside the chartArea, whatever rotation has been set #282

Merged
merged 9 commits into from
Jan 4, 2021
Merged

Maintain the label always inside the chartArea, whatever rotation has been set #282

merged 9 commits into from
Jan 4, 2021

Conversation

stockiNail
Copy link
Collaborator

Fixes #271

@kurkle kurkle closed this Dec 20, 2020
@kurkle kurkle reopened this Dec 20, 2020
@@ -228,7 +228,70 @@ function calculateLabelPosition(line, width, height) {
x = pt.x + xAdjust;
y = pt.y + yAdjust;
}
return {x, y};

a = {x: x - ((width / 2) * Math.cos(angle)) - ((height / 2) * Math.sin(angle)), y: y - ((width / 2) * Math.sin(angle)) + ((height / 2) * Math.cos(angle))};
Copy link
Member

Choose a reason for hiding this comment

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

Only calculating the rotated width/height here, right?

This should work:

const cos = Math.cos(angle);
const sin = Math.sin(angle);
const h = Math.abs(width * sin) + Math.abs(height * cos);
const w = Math.abs(width * cos) + Math.abs(height * sin);

SO:how-to-get-size-of-a-rotated-rectangle

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only calculating the rotated width/height here, right?

Not only, it's calculating all apexes of rotated rectangle, needed to catch also the events correctly.

And then the width and height of the rectangle are calculated using the differences among the apexes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kurkle sorry, I misunderstood your comment, maybe I have understood right now. Let me test it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kurkle in calculateLabelPosition I need to calculate ONLY the width and height and your review is correct. I don't need to calculate the apexes because they must be calculated in adjustLabelPosition.

I think I need to rebase and then I'm gonna push again.

@stockiNail
Copy link
Collaborator Author

@kurkle I was thinking if this PR could still make sense.

The issue #258 could be addressed removing the clip area when the labels are drawn. Therefore it couldn't make sense to try adjusting the drawing of the label inside the chart area if we are thinking to allow this kind of feature.

Having split the drawing of labels from the annotation (#275), the clip area could be maintained only for the annotation and removed for the labels which can be drawn outside of chart area. The used must be take care to create "enough space" (by padding?) in the chart in order to avoid the the label will be cut because is going outside of the canvas.

Nevertheless, if we decided to leave the clip area, in this case this PR looks like very helpful, I guess.
What do you think?

@stockiNail
Copy link
Collaborator Author

What about codeclimate — 8 issues to fix? Shall I try to fix them?
If yes, any hint or can I do as I think to be correct?

@kurkle
Copy link
Member

kurkle commented Jan 3, 2021

If those "similar blocks" look like it would make sense to refactor, take a look at those. The line counts can be ignored for now, we'll probably move something away from here later.

@stockiNail
Copy link
Collaborator Author

I need a little more time to do it, if ok for you. I'm struggling with a case that I didn't recognize before today

@kurkle
Copy link
Member

kurkle commented Jan 3, 2021

No worries, take your time :)

@stockiNail
Copy link
Collaborator Author

If those "similar blocks" look like it would make sense to refactor, take a look at those. The line counts can be ignored for now, we'll probably move something away from here later.

There are only 2 issues related to the lines of code which are exceeding the max allowed.

@kurkle kurkle merged commit a6772ca into chartjs:master Jan 4, 2021
@stockiNail
Copy link
Collaborator Author

@kurkle thank you very much for your support and patience!

@stockiNail stockiNail deleted the fixing-271 branch January 4, 2021 12:32
@kurkle kurkle linked an issue Jan 13, 2021 that may be closed by this pull request
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.

Label is cut due to rotation Label hidden when draw line at top of chart
2 participants