Skip to content
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

Enable fallback to common defaults #745

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented May 23, 2022

As reply to: #740 (comment)

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

@stockiNail
Copy link
Collaborator

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

@kurkle
Copy link
Member Author

kurkle commented May 23, 2022

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
Copy link
Collaborator

stockiNail commented May 23, 2022

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
Copy link
Member Author

kurkle commented May 23, 2022

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
Copy link
Collaborator

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

@kurkle
Copy link
Member Author

kurkle commented May 23, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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 kurkle marked this pull request as draft May 23, 2022 14:22
@kurkle
Copy link
Member Author

kurkle commented May 23, 2022

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

@kurkle
Copy link
Member Author

kurkle commented May 23, 2022

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%)

@stockiNail stockiNail added this to the 2.1.0 milestone Jun 3, 2022
@stockiNail stockiNail modified the milestones: 2.1.0, 2.2.0 Nov 17, 2022
@kurkle kurkle removed this from the 2.2.0 milestone Mar 14, 2023
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.

None yet

2 participants