chartjs-plugin-annotation icon indicating copy to clipboard operation
chartjs-plugin-annotation copied to clipboard

Enable fallback to common defaults

Open kurkle opened this issue 3 years ago • 10 comments

As reply to: https://github.com/chartjs/chartjs-plugin-annotation/pull/740#issuecomment-1134514169

I'm sure I missed some common options, but those can be added in subsequent pull requests.

kurkle avatar May 23 '22 11:05 kurkle

I was testing the same thing, passing the common to updateElements.

stockiNail avatar May 23 '22 12:05 stockiNail

This is not really optimal, feel free to create better alternative:

Filename Size Change
dist/chartjs-plugin-annotation.esm.js 15.5 kB +294 B (+2%)
dist/chartjs-plugin-annotation.js 15.6 kB +298 B (+2%)
dist/chartjs-plugin-annotation.min.js 9.84 kB +102 B (+1%)

kurkle avatar May 23 '22 12:05 kurkle

This is not really optimal, feel free to create better alternative: Filename Size Change dist/chartjs-plugin-annotation.esm.js 15.5 kB +294 B (+2%) dist/chartjs-plugin-annotation.js 15.6 kB +298 B (+2%) dist/chartjs-plugin-annotation.min.js 9.84 kB +102 B (+1%)

I don't know how but I'm gonna think about. ;)

EDIT: Maybe moving more options in common defaults, the size could be decrease, at least close the current size.

stockiNail avatar May 23 '22 12:05 stockiNail

Maybe its tolerable now:

Filename Size Change
dist/chartjs-plugin-annotation.esm.js 15.4 kB +246 B (+2%)
dist/chartjs-plugin-annotation.js 15.5 kB +251 B (+2%)
dist/chartjs-plugin-annotation.min.js 9.8 kB +67 B (+1%)

Would need to do this somehow without deepMerge to make it smaller (and faster probably too)

kurkle avatar May 23 '22 12:05 kurkle

just thinking loud, could we use the proxies created by CHART,JS instead of the simple objects (which needs deep merge)?

stockiNail avatar May 23 '22 12:05 stockiNail

just thinking loud, could we use the proxies created by CHART,JS instead of the simple objects (which needs deep merge)?

I don't recall anymore why we are resolving the options to simple objects. But the proxy probably has a list of all the keys resolvable. Lets see.

kurkle avatar May 23 '22 12:05 kurkle

I'm using your branch locally to do some tests. I can confirm you that it doesn't need to set drawTime options separately.

stockiNail avatar May 23 '22 13:05 stockiNail

Having done some tests, this PR can address to centralized common options in the common but it doesn't solve the issues about duplication of code of callout (for instance). This is because callout node is a subnode of label one (for line) instead for label is a subnode of root one, therefore fallback is not resolved anyway.

stockiNail avatar May 23 '22 14:05 stockiNail

There are some issues still with the fallback definitions, I'll test some more to be sure what's wrong.

kurkle avatar May 23 '22 14:05 kurkle

Filename Size Change
dist/chartjs-plugin-annotation.esm.js 15.2 kB +16 B (0%)
dist/chartjs-plugin-annotation.js 15.3 kB +17 B (0%)
dist/chartjs-plugin-annotation.min.js 9.68 kB -55 B (-1%)

kurkle avatar May 23 '22 14:05 kurkle