chartjs / chartjs-plugin-annotation

Annotation plugin for Chart.js
MIT License
607 stars 328 forks source link

Add `beforeDraw` and `afterDraw` hooks to the annotations #579

Closed stockiNail closed 2 years ago

stockiNail commented 2 years ago

This PR is adding before/after draw hooks in order to allow the user to apply whatever needed customization on out-of the-box annotations.

plugins: {
  annotation: {
    annotations: {
      line: {
        type: 'line',
        ....
        beforeDraw(context) {
          console.log('beforeDraw');
          return true;
        },
        afterDraw(context) {
          console.log('afterDraw');
        },
      },
    }
  }
}
Hook Arguments Return type Default Description
beforeDraw context boolean annotation.beforeDraw invoked before the element drawing. If returns false, the element is not drawn.
afterDraw context void annotation.afterDraw invoked after the element drawing

Common hooks for all annotations can be set in the annotation plugin options:

plugins: {
  annotation: {
    beforeDraw(context) {
      console.log('Common beforeDraw');
      return true;
    },
    afterDraw(context) {
      console.log('Common afterDraw');
    },
    annotations: {
      line: {
        type: 'line',
        ....
      },
    }
  }
}

TODO

stockiNail commented 2 years ago

Due to the fact that element and label couldn't be drawn in sequence but, in accordance with drawTime options, they could be drawn in a difference sequence, some element internal properties, to manage status of drawing, has been added.

kurkle commented 2 years ago

Due to the fact that element and label couldn't be drawn in sequence but, in accordance with drawTime options, they could be drawn in a difference sequence, some element internal properties, to manage status of drawing, has been added.

I asked for only these two hooks first, so I'd see a small PR that is easily reviewed. I think you are trying to implement all the use cases you have with these 2 hooks now?

stockiNail commented 2 years ago

I asked for only these two hooks first, so I'd see a small PR that is easily reviewed. I think you are trying to implement all the use cases you have with these 2 hooks now?

Yes, @kurkle. As shared, I started with before/afterDraw which are including both element and label drawing (as we shared in the other PR). I'm trying to see which use cases I can implement with these 2. For instance, issues 145 and 146 can be addressed by these 2 hooks. Anyway with these hooks you can only extend the existing ones, not create new annotations. To do that, you need more hooks:

  1. before/afterUpdate, to be invoked before and after resolveElementOptions, in order to allow the user to set own dimensions.
  2. inRange, in order to catch events.
  3. centerPoint, maybe not needed if we go to normalized size (issue 532)

That's my opinion and probably I'm wrong!

stockiNail commented 2 years ago

@LeeLenaleee thank you very much! I'm leaving your 2 notes (about comma) open because not clear to me where to add spaces.

stockiNail commented 2 years ago

@kurkle @LeeLenaleee I was thinking about this PR. I think that probably the complexity we are adding here is too much comparing with the benefits.

At the moment, there are 2 issues which can be addressed by this PR but one of them can be also solved implementing shadowing on the annotations and the other one can't be solved as out of scope of the plugin (at least at the moment).

We could freeze this PR and spend more time to think about. What do you think?

LeeLenaleee commented 2 years ago

I don't mind setting this on a hold to see if it might be implemented in a different less complex way. But I do still think it is a verry good addition to the plugin for users to apply a lot of customizability in a relative easy way.

stockiNail commented 2 years ago

I don't mind setting this on a hold to see if it might be implemented in a different less complex way. But I do still think it is a verry good addition to the plugin for users to apply a lot of customizability in a relative easy way.

I agree with you about the capabilities but I wouldn't add complexity if not really needed. In my opinion now it's not so complex but probably the current features are not enough and some other hooks will be needed. Anyway I'm continue thinking to as simple as possible implementation

kurkle commented 2 years ago

Yeah, the "draw state" is a thing I'd like to avoid. Its the only thing I dislike here actually. But lets give it some more time.

kurkle commented 2 years ago

Looks like we could actually release 1.2.2 instead of 1.3.0 without this/other unmerged enhancements.

stockiNail commented 2 years ago

Looks like we could actually release 1.2.2 instead of 1.3.0 without this/other unmerged enhancements.

It makes sense to me!

stockiNail commented 2 years ago

Looks like we could actually release 1.2.2 instead of 1.3.0 without this/other unmerged enhancements.

Do we have to change the milestone label to PR?

stockiNail commented 2 years ago

I know I said to take time to think about but I had item and I remove element._draw status.

Now I'm using 2 flags (_drawElement and _drawLabel) at element level to be aware if the element and the label has been drawn.

stockiNail commented 2 years ago

I need to close this PR because I need to re-fork the project. As soon as I'll have new project, I'll submit new PR with the same content.