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 initial animation on annotations #755

Merged
merged 51 commits into from
Feb 26, 2023
Merged

Enable initial animation on annotations #755

merged 51 commits into from
Feb 26, 2023

Conversation

stockiNail
Copy link
Collaborator

@stockiNail stockiNail commented Jun 1, 2022

This PR is enabling the possibility to the user to set the animation to the annotations when they are drawing at chart initialization.

It adds an options to all annotations: init.

webm.mp4
Option Type Default
init boolean |
(chart, properties, options) => void | boolean | AnnotationBoxModel
false

The callback is not invoked passing the element context (because element is not build in that phase) but receives chart, annotation options and the properties of the element (x, y, x2, y2, centerX, centerY, width, height), calculated before callback invocation.
The callback can return a boolean or an object, with the initial values of the element properties (x, y, x2, y2, centerX, centerY, width, height), needed to perform the animation.

TODO

  • provide 1 default animation (not all modes) and a way to enable the user to customize the default
  • provide the initial animation options at plugin level (for all annotations)
  • test cases
  • types
  • documentation
  • sample
  • wait for Enable label on ellipse annotation #749 approval and merge

@stockiNail
Copy link
Collaborator Author

@kurkle @LeeLenaleee off topic from this PR. Milestone 2.0.0 is growing but I don't know if there is any community best practice to release new major version. Maybe some PRs in queue could be moved to new milestone, 2.1.0 in order to release 2.0.0 to the users.

src/types/point.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Jun 1, 2022

I don't know about best practices, but I'd prefer fast releases. That means trying to include all the breaking changes we think is needed and bug fixes. Non-breaking features can be postponed to next "feature"/minor release.

@stockiNail
Copy link
Collaborator Author

I don't know about best practices, but I'd prefer fast releases. That means trying to include all the breaking changes we think is needed and bug fixes. Non-breaking features can be postponed to next "feature"/minor release.

Great! At the moment I don't see any other breaking change to add. Hopefully I'm right but of course not sure 100%. Therefore some other days are needed, to think about.
Do you think we can create new milestone 2.1.0, in order to move some PRs to that?

@stockiNail
Copy link
Collaborator Author

About CC issue. I can reduce the amount of lines (to recover 2 lines needed) but I think it could be better to split some files of types in some specific files to manage specific topic (label, callout, arrowHeads).

  • Label: move callout management in a specific file
  • Line: move label and arrowHeads management in specific files

This could be addressed by a specific PR.

@kurkle
Copy link
Member

kurkle commented Jun 1, 2022

I don't know about best practices, but I'd prefer fast releases. That means trying to include all the breaking changes we think is needed and bug fixes. Non-breaking features can be postponed to next "feature"/minor release.

Great! At the moment I don't see any other breaking change to add. Hopefully I'm right but of course not sure 100%. Therefore some other days are needed, to think about. Do you think we can create new milestone 2.1.0, in order to move some PRs to that?

Sure we can :)

@kurkle
Copy link
Member

kurkle commented Jun 1, 2022

About CC issue. I can reduce the amount of lines (to recover 2 lines needed) but I think it could be better to split some files of types in some specific files to manage specific topic (label, callout, arrowHeads).

  • Label: move callout management in a specific file
  • Line: move label and arrowHeads management in specific files

This could be addressed by a specific PR.

I think we should switch over to sonar or something else (I don't know anything better), because some of the CC "issues" are nonsense IMO.

src/types/point.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Jun 1, 2022

I'm not sure we should be providing a bunch of builtin initial named animations. I think it would be better to stick what Chart.js does (one default animation), and extend the animation configuration if needed.

@stockiNail
Copy link
Collaborator Author

I'm not sure we should be providing a bunch of builtin initial named animations. I think it would be better to stick what Chart.js does (one default animation), and extend the animation configuration if needed.

Let me recap in order to understand better.
You are proposing 2 things:

  1. to have only 1 animation configuration for the whole plugin (and not at annotation level) and all annotations are acting at the same way in a chart instance
  2. remove all animations modes and provide a default (maybe fade that sounds better to me) but find a way in order to enable the user to enable his/her custom modes.

Have I understood well?

@kurkle
Copy link
Member

kurkle commented Jun 1, 2022

Let me recap in order to understand better. You are proposing 2 things:

  1. to have only 1 animation configuration for the whole plugin (and not at annotation level) and all annotations are acting at the same way in a chart instance

Hmm, no, I don't really have any objection for annotation level animation configuration. Though it should be doable at plugin level with scriptable options too.

  1. remove all animations modes and provide a default (maybe fade that sounds better to me) but find a way in order to enable the user to enable his/her custom modes.

This is what I'm suggesting. I'd rather have samples for some custom animations instead of built-in modes.

@stockiNail
Copy link
Collaborator Author

Let me recap in order to understand better. You are proposing 2 things:

  1. to have only 1 animation configuration for the whole plugin (and not at annotation level) and all annotations are acting at the same way in a chart instance

Hmm, no, I don't really have any objection for annotation level animation configuration. Though it should be doable at plugin level with scriptable options too.

  1. remove all animations modes and provide a default (maybe fade that sounds better to me) but find a way in order to enable the user to enable his/her custom modes.

This is what I'm suggesting. I'd rather have samples for some custom animations instead of built-in modes.

Clear!!! Let me code a little bit, going to this direction. I have to think a clever way to provide a customization method to the users.
I'm going to leave this PR in draft anyway. ;)

@stockiNail
Copy link
Collaborator Author

I think we should switch over to sonar or something else (I don't know anything better), because some of the CC "issues" are nonsense IMO.

I have experience on Sonar but for JAVA code (my project is there). But I don't have enough experience to say if Sonar is better than CC for JS. Maybe there some good things in CC comparing with Sonar that I don't know

@stockiNail stockiNail added this to the 2.0.0 milestone Jun 1, 2022
@stockiNail
Copy link
Collaborator Author

@kurkle I think I have completed the "first" imp, following your hints. I don't release this PR because it needs to be changed when other PRs will be merged (mainly the label on ellipse). Anyway, feel free to comment and review it, I'll change the impl accordingly.

@stockiNail
Copy link
Collaborator Author

Sure we can :)

done!

@stockiNail stockiNail modified the milestones: 2.0.0, 2.1.0 Jun 3, 2022
@kurkle
Copy link
Member

kurkle commented Jan 26, 2023

Correct me if I'm wrong, but does initAnimations provide the starting properties for annotation animations?
If that is it, I think it could be named better. Maybe initialState would describe it better?
Or maybe it could just initialize or even init.

@stockiNail
Copy link
Collaborator Author

Correct me if I'm wrong, but does initAnimations provide the starting properties for annotation animations?

Yes , it does.

If that is it, I think it could be named better. Maybe initialState would describe it better? Or maybe it could just initialize or even init.

OK, I'll do. I have named it adding Animations because I wanted to highlight we are talking about init of animation.

@stockiNail
Copy link
Collaborator Author

If that is it, I think it could be named better. Maybe initialState would describe it better?
Or maybe it could just initialize or even init.

Renamed in init.

Conflicts:
	docs/guide/configuration.md
	src/helpers/helpers.options.js
	src/types/line.js
	types/options.d.ts
kurkle
kurkle previously approved these changes Feb 24, 2023
@kurkle
Copy link
Member

kurkle commented Feb 24, 2023

Needs a rebase

@stockiNail
Copy link
Collaborator Author

Merging, helpers.canvas has got 254 rows, more than 250 set by CC... :(

@stockiNail stockiNail merged commit 48d87cd into chartjs:master Feb 26, 2023
@stockiNail stockiNail deleted the initAnimation branch February 26, 2023 00:14
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