search

chartjs / chartjs-plugin-annotation

Annotation plugin for Chart.js
MIT License
603 stars 325 forks source link

Enable fallback to common defaults #745

Open kurkle opened 2 years ago

kurkle commented 2 years ago

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.

stockiNail commented 2 years ago

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

kurkle commented 2 years ago

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%)
stockiNail commented 2 years ago

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.

kurkle commented 2 years ago

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)

stockiNail commented 2 years ago

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

kurkle commented 2 years ago

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.

stockiNail commented 2 years ago

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 commented 2 years ago

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.

kurkle commented 2 years ago

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

kurkle commented 2 years ago
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%)