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.

Sign up for GitHub

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

Jump to bottom

Enable line annotation label as label sub-element #727

Merged
merged 12 commits into from
May 16, 2022
Merged

Enable line annotation label as label sub-element #727

merged 12 commits into from
May 16, 2022

Conversation

stockiNail
Copy link
Collaborator

This PR moves label node of line annotation to sub element.

@stockiNail stockiNail added this to the 2.0.0 milestone Apr 26, 2022
@stockiNail stockiNail requested a review from kurkle April 26, 2022 19:04
kurkle
kurkle reviewed Apr 26, 2022
View reviewed changes
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.

Took just a quick look on phone. I will try to review properly some other day.

src/elements.js Outdated Show resolved Hide resolved
@stockiNail stockiNail requested a review from kurkle May 9, 2022 10:48
kurkle
kurkle reviewed May 12, 2022
View reviewed changes
src/annotation.js Outdated Show resolved Hide resolved
src/elements.js Outdated Show resolved Hide resolved
test/fixtures/line/labelShadowColors.js Show resolved Hide resolved
@stockiNail stockiNail changed the title Enable line annotation label as label sub-element Enable line annotation label as label sub-element - old May 13, 2022
@stockiNail stockiNail changed the title Enable line annotation label as label sub-element - old Enable line annotation label as label sub-element May 13, 2022
@stockiNail stockiNail requested a review from kurkle May 13, 2022 10:35
kurkle
kurkle reviewed May 14, 2022
View reviewed changes
src/types/label.js Outdated Show resolved Hide resolved
kurkle
kurkle previously approved these changes May 14, 2022
View reviewed changes
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.

Not sure why this.rotation and this.options.rotation need both to be used (and the fallback with || might not work in all cases as expected, specifically when this.rotration is 0)

@stockiNail
Copy link
Collaborator Author

Not sure why this.rotation and this.options.rotation need both to be used (and the fallback with || might not work in all cases as expected, specifically when this.rotration is 0)

I thought a little bit more and you're right! This is wrong only when the calculated rotation is 0 and the label options of a line is set to 'auto'.

Maybe the best could be to set element.rotation for all annotations which have got a label, and then use it instead of the options.

@stockiNail
Copy link
Collaborator Author

Not sure why this.rotation and this.options.rotation need both to be used (and the fallback with || might not work in all cases as expected, specifically when this.rotration is 0)

@kurkle I have changed using only element.rotation at runtime of the label.

@stockiNail stockiNail merged commit 7525265 into chartjs:master May 16, 2022
@stockiNail stockiNail deleted the subelementLineLabel branch May 16, 2022 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kurkle kurkle kurkle approved these changes

Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@stockiNail @kurkle