-
Notifications
You must be signed in to change notification settings - Fork 328
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
Implement a separate labelDrawTime option #275
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to use drawTime
options into label
node instead of labelDrawTime
at option level.
README.md
Outdated
@@ -30,6 +30,10 @@ To configure the annotations plugin, you can simply add new config options to yo | |||
// See http://www.chartjs.org/docs/#advanced-usage-creating-plugins | |||
drawTime: 'afterDatasetsDraw', // (default) | |||
|
|||
// Defines when annotations' labels are drawn. Defaults to | |||
// drawTime. | |||
labelDrawTime: 'afterDatasetsDraw', // (default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshkel may I ask you why to use a property at element level instead of using drawTime
property at label
node?
src/annotation.js
Outdated
} | ||
}); | ||
elements.forEach(el => { | ||
if ('drawLabel' in el && (el.options.labelDrawTime || el.options.drawTime || options.labelDrawTime || options.drawTime || caller) === caller) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshkel using drawTime
at label
level, can the statement be the following?
if ('drawLabel' in el && (el.options.label.drawTime || el.options.drawTime || options.drawTime || caller) === caller) {
Thanks for the suggestion, @stockiNail. I hadn't considered using a I noticed that text annotations were listed under "To-do Items" (and it's probably something that our project will eventually need). I'm guessing that a hypothetical text annotations feature would use the same |
@joshkel you would like to set the label
I was thinking about that (also to have label on the box and all other annotation types) and maybe an abstract label class is needed and every annotation could extend and implement special labeling. |
@joshkel apologize, I forgot an important thing on my previous answer. Therefore if you want to configure all line annotations (but this is valid for all annotation types) with a specific options: {
elements: {
lineAnnotation: {
drawTime: 'beforeDatasetsDraw',
label: {
drawTime: 'afterDraw'
},
}
....
plugins: {
annotation: {
annotations: {
myHorizontalLine: {
type: 'line',
scaleID: 'y',
value: -90,
label: {
position: 'center',
font: {
size: 16,
},
backgroundColor: 'red',
content: 'This is a label',
enabled: true
}
},
}
}
}
...
} You can do the same into In this way you can configure the Unfortunately a piece of code is currently missing into plugin but I think it makes sense to add: el.options = merge(Object.create(null), [elType.defaults, chart.options.elements[elType.id], annotation]); This line is of my branch where the options are calculated differently for your code (when merged and after conflicts resolving, it should work changing that). In this way you can get the If you agree on that (to use |
I think I like the I'm planning to provide some utilities for this in the core, no need to write the resolution path in every plugin (and extension):
|
Sounds good! |
Apologies for the delayed reply. I changed to Do you want me to do anything with merging options now, or wait until other PRs and/or core work? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the beginPath() and merging README.md with master, this seems good to go.
Implementation notes: Since not all annotation types implement labels, the `drawLabel` method is optional. If the `labelDrawTime` option isn't specified, then it checks the individual annotation's `drawTime` option, then the chart-wide annotation plugin's `labelDrawTime` option, then the chart-wide annotation plugin's `drawTime` option. This means that you can't override an individual annotation's `drawTime` without also affecting its `labelDrawTime`, but I think it's the more intuitive and backward-compatible behavior. Since the option should default to `drawTime` if not set, I did not add it to the `defaults` object. Please let me know if I've misunderstood how plugins' default options should be implemented.
As suggested in code review.
0713f07
to
78e4add
Compare
From what I understand, if we want to allow registering additional element options (see [here][1] for an example), then the element options need to be a top-level interface so that they can be used with TypeScript's [declaration merging][2]. [1]: chartjs/chartjs-plugin-annotation#275 (comment) [2]: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
From what I understand, if we want to allow registering additional element options (see [here][1] for an example), then the element options need to be a top-level interface so that they can be used with TypeScript's [declaration merging][2]. [1]: chartjs/chartjs-plugin-annotation#275 (comment) [2]: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
From what I understand, if we want to allow registering additional element options (see [here][1] for an example), then the element options need to be a top-level interface so that they can be used with TypeScript's [declaration merging][2]. [1]: chartjs/chartjs-plugin-annotation#275 (comment) [2]: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
Implementation notes:
Since not all annotation types implement labels, the
drawLabel
method is optional.If the
labelDrawTime
option isn't specified, then it checks the individual annotation'sdrawTime
option, then the chart-wide annotation plugin'slabelDrawTime
option, then the chart-wide annotation plugin'sdrawTime
option. This means that you can't override an individual annotation'sdrawTime
without also affecting itslabelDrawTime
, but I think it's the more intuitive and backward-compatible behavior.Since the option should default to
drawTime
if not set, I did not add it to thedefaults
object. Please let me know if I've misunderstood how plugins' default options should be implemented.