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

Fix #10749 - backdrops with rotated labels #10759

Merged
merged 3 commits into from
Oct 12, 2022

Conversation

cmcnulty
Copy link
Contributor

@cmcnulty cmcnulty commented Oct 6, 2022

In order to make the backdrop work on rotated labels, the backdrop needs to be moved into renderText(). Additionally, the x/y coordinates calculated inside _computeLabelItems() didn't properly account for rotated text, and the most elegant solution turned out to be to just use modern measureText() to determine the x/y of the label.

Although the label backdrop isn't a very used feature, probably, storing the accurate coords inside of the options group opens up a range of possibilities with attaching hover/click events over the axis labels, which are otherwise impossible.

test/fixtures/core.scale/tick-backdrop.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
Because backdrop now occurs after translation, we don't want to double-adjust the position.
@cmcnulty
Copy link
Contributor Author

@etimberg , @kurkle , I appreciate the feedback - I went back and figured out what was really going on - because I moved the backdrop after the rotation (in order to address the bug), the translation adjustment was being double-counted. With my most recent commit, it simply removes the top/left adjustment, because that's now taken care of in the translation. I'd appreciate another look. Thank you so much for your time!!

etimberg
etimberg previously approved these changes Oct 11, 2022
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.

@cmcnulty Thank you! That looks more like what I had imagined would be needed

@etimberg etimberg merged commit fbf3427 into chartjs:master Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants