Move element fallback to plugins.annotation.common

[kurkle]

Fix #625

Alternative approach to this isssue. I like this better than #629, but this is a breaking change (most notably the drawTime option not read from plugins.annotation.drawTime as fallback any more)

Noticed the drawTime fixture was not tested relative to dataset, so added some data to that.

[stockiNail]

@kurkle probably the documentation about the common configuration (/~https://github.com/chartjs/chartjs-plugin-annotation/blob/master/docs/guide/configuration.md) should be changed.

I'm going to test it but the events callbacks will stay at annotation level, doesn't it?

[kurkle]

@kurkle probably the documentation about the common configuration (/~https://github.com/chartjs/chartjs-plugin-annotation/blob/master/docs/guide/configuration.md) should be changed.

Yes, I wanted to see what you think about this before updating docs. Not sure about the common name, alternatives welcomed.

I'm going to test it but the events callbacks will stay at annotation level, doesn't it?

Yes, the plugin options stay at that level, only the element fallback options are nested.

[stockiNail]

It sounds good to me.
Another doubt: why is not animations moved in the common? For the same reason of events callbacks?

off-topic: could common contain also the "common options" defaults among all elements (like shadow or x/yScaleID, x/yMin or x/yMax?

[kurkle]

It sounds good to me. Another doubt: why is not animations moved in the common? For the same reason of events callbacks?

Animations are only resolved from plugin options, elements can't (currently) overwrite that. It can be changed if we want to configure the animations per element type, but I don't think we have real use cases for that (yet).

off-topic: could common contain also the "common options" defaults among all elements (like shadow or x/yScaleID, x/yMin or x/yMax?

Yes, that has been the original idea, but it needs a little more work. Currently we are using the "keys" from element defaults as the list of properties to resolve. So we could add the common keys to that list and would not need to have those common keys listed in element defaults at all.

What do you think about shared as the name instead of common?

[stockiNail]

What do you think about shared as the name instead of common?

personally I would prefer common. I have never seen also in other projects a label shared but both terms are explaining well the meaning of the that node, I guess.

[stockiNail]

So we could add the common keys to that list and would not need to have those common keys listed in element defaults at all.

This is something that I haven't understood yet. For all tests that I have done, if you don't set the key as undefined, the fallback doesn't occur.

[kurkle]

So we could add the common keys to that list and would not need to have those common keys listed in element defaults at all.

This is something that I haven't understood yet. For all tests that I have done, if you don't set the key as undefined, the fallback doesn't occur.

This is where the keys are read from defaults object:

chartjs-plugin-annotation/src/annotation.js

Lines 187 to 191 in d999608

	for (const name of Object.keys(defs)) {
	const optDefs = defs[name];
	const value = resolver[name];
	result[name] = isObject(optDefs) ? resolveObj(value, optDefs) : value;
	}

[stockiNail]

Ok, therefore the keys must be defined. In teh label annotation, there is a useless defaultRoutes in the background color because the default ('transparent') is set in the defaults.

[stockiNail]

@kurkle I think it could be helpful to add an additional option (enabled) at annotation node in order to disable the plugin without loosing the whole plugin configuration.

This PR will add a common defaults for all annotations and it can contain the display for all, in order that the user can disable all the annotations changing that. I see only 2 concern:

1. the plugin will perfom all calculation and operation even if not needed
2. it sounds like a different way comparing with other plugins (like title, legend, subtitle)

What do you think?

[stockiNail]

@kurkle I had time now to test if this PR is solving the issue #625 and unfortunately it doesn't seem so, because if I config a label annotation with label as id, the annotation is not shown