Skip to content

Add support for per-element clip attributes #972

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented Apr 16, 2025

Fixes #874

I see in #874 that there are potential backwards compatibility concerns. I'm not aware of any in this code as implemented, but I still feel like I don't necessarily understand Chart.js's options implementation, so I may be missing something.

If this feature and approach seems desirable, I'll update tests and docs. Thank you.

@stockiNail
Copy link
Collaborator

@joshkel thank you very much! I don't recall why we had some doubts to implement clip option at element level (my memory is not so persistent but it could be related to performance issue?!?).
For me it makes sense.

Nevertheless, your PR sounds good even if it would be perfect to move the clip option in the annotation plugin to the common node.
Why? The common node is the fallback node for element defaults. Which means that if statements in your code will be useless because we could only check item.element.options.clip. If not set in the element, it will have the value of the common.

That was the idea at that time when we talked about that. But it is also clear, my proposal would be a breaking change and therefore it will need a major version.

@joshkel
Copy link
Contributor Author

joshkel commented Apr 17, 2025

The purpose of the if checks is to limit performance impact: if a portion of the list of annotation elements all has the same clip option, then the plugin can avoid continually applying then clearing clipping settings.

Regarding whether to put the option in common: I'm afraid I don't understand why adding it there would be a breaking change. But I'm happy to go whichever route you prefer.

@stockiNail
Copy link
Collaborator

Regarding whether to put the option in common: I'm afraid I don't understand why adding it there would be a breaking change. But I'm happy to go whichever route you prefer.

Because I mean "moving" and not "adding", because only adding it, you will have the clip on the root config node which is not used.
And moving it, whoever is using clip should change the own implementation.

@stockiNail
Copy link
Collaborator

Nevertheless I think we could proceed step by step, and going on with this implementation and in the new major, change it.

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.

Configurable clip for each annotation
2 participants