chartjs / chartjs-plugin-annotation

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

Add custom annotation type #524

Closed kurkle closed 2 years ago

kurkle commented 2 years ago

Maybe the sample could be a bit more complex:

image

stockiNail commented 2 years ago

GREAATT! Just a couple of doubts:

  1. there is the "box" location. I very often see that "point" is more comfortable. Maybe it could be implemented as well. I meant xValue, yValue.
  2. how the custom annotation can receive "own" options? as far as I have seen (maybe I'm wrong) adding your options to the config, are NOT available (I think it's related to the resolver, in the annotation.js)
stockiNail commented 2 years ago
  1. there is the "box" location. I very often see that "point" is more comfortable. Maybe it could be implemented as well. I meant xValue, yValue.

Generally, I was thinking this afternoon that, apart for box and line annotations, all other could be drawn both with box and point location.

kurkle commented 2 years ago
  1. there is the "box" location. I very often see that "point" is more comfortable. Maybe it could be implemented as well. I meant xValue, yValue.
  2. how the custom annotation can receive "own" options? as far as I have seen (maybe I'm wrong) adding your options to the config, are NOT available (I think it's related to the resolver, in the annotation.js)
  1. I'll base that on your work in #527
  2. Implemented a customOptions option for that!
stockiNail commented 2 years ago

2. Implemented a customOptions option for that!

Greeaat!! I'm looking forward to use it. Anyway I'll continue having a look to the implementation but I don't want to disturb you. I'll submit comments ...

stockiNail commented 2 years ago

@kurkle how did you solve the setting of global defaults for customAnnotation? Probaly the customAnnotation can not be configured a global level because more custom implementations could refer to the same default element.

kurkle commented 2 years ago

@kurkle how did you solve the setting of global defaults for customAnnotation? Probaly the customAnnotation can not be configured a global level because more custom implementations could refer to the same default element.

I did not really give any thought to that before you asked. I'd expect full control of the custom annotations on a project level, so it's possible to use different option names for things that need to have different global defaults. I don't really see a problem there, but maybe you have some problematic use cases?

stockiNail commented 2 years ago

I don't really see a problem there, but maybe you have some problematic use cases?

At the moment I don't have any specific use case.

I'd expect full control of the custom annotations on a project level, so it's possible to use different option names for things that need to have different global defaults.

I was thinking the opposite, more projects where each one ca provide own custom annotation. Something like the controllers.

Nevertheless I agree that's not a showstopper. Using a property inside a node could be enough. And then we will see if specific use case will pop up.

stockiNail commented 2 years ago

I know you are still in "development" phase and maybe you are not thinking about that, but if I may:

  1. the label annotation is an example how the content of the annotation is affecting the size of the element. Maybe this use case could be also valid for custom implementation and therefore there could be a init property in the options (like draw) which can be invoked to redefine the element size.
  2. There are many invocation of draw function during the lifecycle of the plugin. As is today in many annotations, we could needd to store a property with a status or other (see labelRect) but i don't have any real object in the arguments where do it:
    options.draw(ctx, {x, y, width, height}, options, this._chart);

As for a context and scriptable options, instead of passing new object ({x, y, width, height})every time, it could be better to store in the annotation element. In this way, the user can add to it all is needed and find it again in the next execution.

This topic recall me that there is another PR to submit, about the event invocation, without creating new context every time:

https://github.com/chartjs/chartjs-plugin-annotation/blob/b9e47c1903900c6c9614bf23ff70213dd3caf0fe/src/events.js#L112-L114

I think should be enough to element.$context, if element is consistent..

kurkle commented 2 years ago

I was thinking the opposite, more projects where each one ca provide own custom annotation. Something like the controllers.

You are thinking shared custom annotations? I thought this would be an very simple way of getting an custom annotation need solved without finalizing and distributing it as a package. Essentially you only need to write the drawing part and there are lots of snippets around to start from. The doubt I have with enabling custom annotations as plugin to this plugin, is that as there aren't so many direct (maintained) plugins around, will there ever be a plugin for a plugin?

Another consideration is the amount of work needed to get a custom annotation done. When you only have to write the draw function and are handed a way to get a point or rect to start from, how could it be easier?

And yet another point, if some annotation type is considered valuable enough, that it should be shared to the community, I think it should be welcomed and added directly here.

As always, these are just my thoughts and debatable :)

kurkle commented 2 years ago

I know you are still in "development" phase and maybe you are not thinking about that, but if I may:

Thanks, valuable thoughts! I'll try to take these into account.

kurkle commented 2 years ago

This topic recall me that there is another PR to submit, about the event invocation, without creating new context every time

Initially I left it that way and thought it can be addressed when the use case with performance problems arises. Forgot about it since though and optimization is never waste, so looking forward to it!

stockiNail commented 2 years ago

The doubt I have with enabling custom annotations as plugin to this plugin, is that as there aren't so many direct (maintained) plugins around, will there ever be a plugin for a plugin? You are thinking shared custom annotations? I thought this would be an very simple way of getting an custom annotation need solved without finalizing and distributing it as a package. Essentially you only need to write the drawing part and there are lots of snippets around to start from. The doubt I have with enabling custom annotations as plugin to this plugin, is that as there aren't so many direct (maintained) plugins around, will there ever be a plugin for a plugin?

Another consideration is the amount of work needed to get a custom annotation done. When you only have to write the draw function and are handed a way to get a point or rect to start from, how could it be easier?

And yet another point, if some annotation type is considered valuable enough, that it should be shared to the community, I think it should be welcomed and added directly here.

As always, these are just my thoughts and debatable :)

Everything you wrote is completely true and shareable. Your thoughts are more valuable than debatable! Believe me! In fact, I thank you because sometimes I take a bribe for unhealthy ideas, like this one. I'm with you, 100%! Let's continue with this implementation that you will see will solve the needs of many, quickly and easily.

kurkle commented 2 years ago

Should probably allow overriding inRange too.

stockiNail commented 2 years ago

Should probably allow overriding inRange too.

In my opinion, yes! Finally we should need init (invokable from revolseElementOptions), draw (from draw) and inRange (from in Range).

stockiNail commented 2 years ago

I was thinking about the current issues where custom annotation could be used to address the enhancement.

I see 2 possible existing use cases: #258 and #146. I have started from #146 and how the user could implement the custom annotation. The init sounds mandatory in order the user can set the width, exceeding the chart area. And then the draw to draw the line. I have stopped myself here because probably some helpers function could help the user to draw the line.

kurkle commented 2 years ago

If you look at the sample image closely, you see that I also had #146 in mind

kurkle commented 2 years ago

Also if the user does not need to interact with the annotation, init and inRange are not needed

stockiNail commented 2 years ago

Also if the user does not need to interact with the annotation, init and inRange are not needed

Yes, nevertheless it could make sense to have them I think.

stockiNail commented 2 years ago

@kurkle yesterday I was thinking if the hooks you added here could be helpful also to current annotations, in order to enrich or change a little the current their behavior.

For instance, #146 needs to draw the line till the end f the canvas. Having the init hook, the user could change only the width of the element in the init and nothing to draw.

And beforeDraw and afterDraw hooks in the existing annotations in order to be able to add additional items. For instance you could use beforeDraw to draw the shadow. The afterDraw could add additional items (for instance to draw a point in the label annotation to have something similar to a sticky note).

stockiNail commented 2 years ago

I have done an experiment to address #146 , adding init option to line annotation:

Here the config:

options: {
  layout: {
    padding: {
      left: 0,
      right: 200,
      top: 0,
      bottom: 0
    },
  },
  ...
  plugins: {
    annotation: {
      clip: false,
      annotations: {
      myHorizontalLine: {
        type: 'line',
        scaleID: 'y',
        value: 25,
        borderColor: 'black',
        borderWidth: 5,
        label: {
          position: 'start',
          yAdjust: -20,
          backgroundColor: 'red',
          content: 'This is a long test label',
          enabled: true
        },
        init(rect, chart) {
          rect.x2 = chart.canvas.width - rect.x;
          rect.width = rect.x2 - rect.x;
          return rect;
        }
      }
    }
  }  
}

Result:

customAnn