-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Check colors definition in the chart options #11003
Conversation
Co-authored-by: Dan Onoshko <[email protected]>
Co-authored-by: Dan Onoshko <[email protected]>
Co-authored-by: Dan Onoshko <[email protected]>
@dangreen Thank you for review! Applied and a bit changed (options is the reference to plugin options, provided to the hook). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still room for improvement after this.
This could be checked by iterating the chart.configdatasetElementScopeKeys(type, elType)
But that would still leave out the defaults.datasets and defaults.elements etc.
Yet another option could be to use the dataset options resolver, but also check if the value is the value of defaults.color
and consider it undefined in that case too.
IMO best option overall would be to improve the options resolver to be able to tell you where the option is resolved from.
It seems fine to me, but 2 functions with almost same name is a bit odd: containsColorsDefinitions, containsColorsDefinition. |
@kurkle Can I try to implement it or do you want to do it? |
It can be done later, if ever. This should be ok for most users already. |
yes, not for this PR. I'm curious to understand how to expose a method (or whatever else) to a Proxy in order to get the scope where the prop is resolved. |
@kurkle just FYI (just a prototype): Added a property to the cache of the proxy _search: () => function(key) {
for (const scope of scopes) {
if (!scope) {
continue;
}
const value = scope[key];
if (defined(value)) {
return scope;
}
}
}, chart.options._search('backgroundColor'); // ==> returns the scope, for instance Chart.defaults. |
Looks simple enough 😃 |
This is more complex.... The "name" should be stored when a scope is calculated but in a different object (I don't think we could add a property to the scope or maybe yes with a special value, i.e. "_name"). We could also "wrap" the scope in an object, with a name property, but this could be quite disruptive, where the scopes are created and used. Let me spend some days to have a look. |
I think it could be a WeakMap usink the scope object as key and name as value. |
yes, even if I need to understand how to relate the key (scope) to the chart in order to keep the map "updated" otherwise I see the risk that it can grow maintaining useless reference. But not sure about that. EDIT: That is, an object's presence as a key in a WeakMap does not prevent the object from being garbage collected. Once an object used as a key has been collected, its corresponding values in any WeakMap become candidates for garbage collection as well — as long as they aren't strongly referred to elsewhere. |
Fix #11002