From 43e71787a496c2926505977c3a57cfe06a1aefb8 Mon Sep 17 00:00:00 2001 From: stockiNail Date: Wed, 6 Apr 2022 18:08:45 +0200 Subject: [PATCH 1/4] Remove dblclick event hook to prepare chart redrawing --- docs/guide/configuration.md | 2 -- src/annotation.js | 1 - src/events.js | 21 ++------------------- test/events.js | 25 ------------------------- test/specs/events.spec.js | 23 ----------------------- types/events.d.ts | 1 - types/options.d.ts | 1 - 7 files changed, 2 insertions(+), 72 deletions(-) diff --git a/docs/guide/configuration.md b/docs/guide/configuration.md index 7007dfa86..b53890ec0 100644 --- a/docs/guide/configuration.md +++ b/docs/guide/configuration.md @@ -10,7 +10,6 @@ The following options are available at the top level. They apply to all annotati | ---- | ---- | :----: | ---- | ---- | [`animations`](#animations) | `object` | No | [see here](#default-animations) | To configure which element properties are animated and how. | `clip` | `boolean` | No | `true` | Are the annotations clipped to the chartArea. -| `dblClickSpeed` | `number` | Yes | `350` | Time to detect a double click in ms. | `drawTime` | `string` | Yes | `'afterDatasetsDraw'` | See [drawTime](options#draw-time). | [`interaction`](options#interaction) | `Object` | No | `options.interaction` | To configure which events trigger plugin interactions @@ -65,6 +64,5 @@ The following options are available for all annotation types. These options can | Name | Type | [Scriptable](options#scriptable-options) | Notes | ---- | ---- | :----: | ---- | `click` | `(context, event) => void` | No | Called when a single click occurs on the annotation. -| `dblClick` | `(context, event) => void` | No | Called when a double click occurs on the annotation. | `enter` | `(context, event) => void` | No | Called when the mouse enters the annotation. | `leave` | `(context, event) => void` | No | Called when the mouse leaves the annotation. diff --git a/src/annotation.js b/src/annotation.js index d03b9c318..bc22abf37 100644 --- a/src/annotation.js +++ b/src/annotation.js @@ -120,7 +120,6 @@ export default { }, }, clip: true, - dblClickSpeed: 350, // ms drawTime: 'afterDatasetsDraw', interaction: { mode: undefined, diff --git a/src/events.js b/src/events.js index f59b9083f..f5e13edec 100644 --- a/src/events.js +++ b/src/events.js @@ -1,7 +1,7 @@ import {defined, callback} from 'chart.js/helpers'; import {getElements} from './interaction'; -const clickHooks = ['click', 'dblclick']; +const clickHooks = ['click']; const moveHooks = ['enter', 'leave']; export const hooks = clickHooks.concat(moveHooks); @@ -93,24 +93,7 @@ function handleClickEvents(state, event, options) { const listeners = state.listeners; const elements = getElements(state, event, options.interaction); for (const element of elements) { - const elOpts = element.options; - const dblclick = elOpts.dblclick || listeners.dblclick; - const click = elOpts.click || listeners.click; - if (element.clickTimeout) { - // 2nd click before timeout, so its a double click - clearTimeout(element.clickTimeout); - delete element.clickTimeout; - dispatchEvent(dblclick, element, event); - } else if (dblclick) { - // if there is a dblclick handler, wait for dblClickSpeed ms before deciding its a click - element.clickTimeout = setTimeout(() => { - delete element.clickTimeout; - dispatchEvent(click, element, event); - }, options.dblClickSpeed); - } else { - // no double click handler, just call the click handler directly - dispatchEvent(click, element, event); - } + dispatchEvent(element.options.click || listeners.click, element, event); } } diff --git a/test/events.js b/test/events.js index 5bc1d1083..9cd7d9924 100644 --- a/test/events.js +++ b/test/events.js @@ -99,31 +99,6 @@ export function testEvents(options) { }); window.triggerMouseEvent(chart, 'click', getCenterPoint(chart)); }); - - it('should detect dbl click event', function(done) { - const dblClickSpy = jasmine.createSpy('dblclick'); - - targetOptions.dblclick = dblClickSpy; - pluginOpts.dblClickSpeed = 1000; - pluginOpts.annotations = [options]; - - const chart = window.acquireChart(chartConfig); - const eventPoint = getCenterPoint(chart); - - let dblClick = false; - window.afterEvent(chart, 'click', function() { - if (!dblClick) { - dblClick = true; - window.triggerMouseEvent(chart, 'click', eventPoint); - } else { - expect(dblClickSpy.calls.count()).toBe(1); - delete targetOptions.dblclick; - delete pluginOpts.dblClickSpeed; - done(); - } - }); - window.triggerMouseEvent(chart, 'click', eventPoint); - }); }); }); } diff --git a/test/specs/events.spec.js b/test/specs/events.spec.js index 95574e39f..23a21c0be 100644 --- a/test/specs/events.spec.js +++ b/test/specs/events.spec.js @@ -93,28 +93,5 @@ describe('Common', function() { }); }); }); - - it('should detect a click event even if 2 clicks are fired', function(done) { - const dblClickSpy = jasmine.createSpy('dblclick'); - annotation.dblclick = dblClickSpy; - - const chart = window.scatterChart(10, 10, {annotation}); - const eventPoint = window.getCenterPoint(chart); - - let dblClick = false; - window.afterEvent(chart, 'click', function() { - if (!dblClick) { - dblClick = true; - setTimeout(() => { - window.triggerMouseEvent(chart, 'click', eventPoint); - }, 500); - } else { - expect(dblClickSpy.calls.count()).toBe(0); - delete annotation.dblclick; - done(); - } - }); - window.triggerMouseEvent(chart, 'click', eventPoint); - }); }); }); diff --git a/types/events.d.ts b/types/events.d.ts index f2043c115..4d8022018 100644 --- a/types/events.d.ts +++ b/types/events.d.ts @@ -23,5 +23,4 @@ export interface AnnotationEvents { enter?(context: EventContext, event: ChartEvent): void, leave?(context: EventContext, event: ChartEvent): void, click?(context: EventContext, event: ChartEvent): void, - dblclick?(context: EventContext, event: ChartEvent): void, } diff --git a/types/options.d.ts b/types/options.d.ts index e1d645519..5d7af8f41 100644 --- a/types/options.d.ts +++ b/types/options.d.ts @@ -155,7 +155,6 @@ export interface AnnotationPluginOptions extends AnnotationEvents { animations?: Record, annotations: AnnotationOptions[] | Record, clip?: boolean, - dblClickSpeed?: Scriptable, drawTime?: Scriptable, interaction?: CoreInteractionOptions } From 4ad82d87eafe53d9b19bab8f33cfb915753218b8 Mon Sep 17 00:00:00 2001 From: stockiNail Date: Wed, 6 Apr 2022 18:22:58 +0200 Subject: [PATCH 2/4] adds note about breaking change to migration guide --- docs/guide/migrationV2.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/guide/migrationV2.md b/docs/guide/migrationV2.md index 3ecb25a2c..01c473c49 100644 --- a/docs/guide/migrationV2.md +++ b/docs/guide/migrationV2.md @@ -12,9 +12,12 @@ A number of changes were made to the configuration options passed to the plugin * `cornerRadius` option was replaced by `borderRadius` in the box annotation configuration and in the label configuration of line annotation to align with Chart.js options. * `xPadding` and `yPadding` options were merged into a single `padding` object in the label configuration of line annotation to align with Chart.js options. * `enabled` option was replaced by `display` in the callout configuration of label annotation, in the label configuration of line and box annotations and in the arrow heads configuration of line annotation to have the same option on all elements. + * `dblClickSpeed` option was removed from the plugin options because `dblclick` event hook is not available anymore. ## Events `chartjs-plugin-annotation` plugin version 2 introduces the [`interaction`](options#interaction) options, to configure which events trigger annotation interactions. By default, the plugin uses the [chart interaction configuration](https://www.chartjs.org/docs/latest/configuration/interactions.html#interactions). * When [scatter charts](https://www.chartjs.org/docs/latest/charts/scatter.html) are used, the interaction default `mode` in Chart.js is `point`, while, in the previous plugin version, the default was `nearest`. + +The `dblclick` event hook was removed from annotations options because, being executed asynchronously, it can not enable the chart re-rendering, automatically after processing the event completely. This is important when the user requires re-draws. It gets slow and messy if every event hook does the draw (or update!). From 9e999a3292e41609a0620dda0e13a0082ba5ddb0 Mon Sep 17 00:00:00 2001 From: stockiNail Date: Wed, 6 Apr 2022 18:29:09 +0200 Subject: [PATCH 3/4] removes click hooks array not needed anymore --- src/events.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/events.js b/src/events.js index f5e13edec..bd47efe5b 100644 --- a/src/events.js +++ b/src/events.js @@ -1,9 +1,8 @@ import {defined, callback} from 'chart.js/helpers'; import {getElements} from './interaction'; -const clickHooks = ['click']; const moveHooks = ['enter', 'leave']; -export const hooks = clickHooks.concat(moveHooks); +export const hooks = moveHooks.concat('click'); export function updateListeners(chart, state, options) { state.listened = false; @@ -27,11 +26,9 @@ export function updateListeners(chart, state, options) { if (!state.listened || !state.moveListened) { state.annotations.forEach(scope => { if (!state.listened) { - clickHooks.forEach(hook => { - if (typeof scope[hook] === 'function') { - state.listened = true; - } - }); + if (typeof scope.click === 'function') { + state.listened = true; + } } if (!state.moveListened) { moveHooks.forEach(hook => { From 3802116f2b2271122fdacbbe768d0bb28351cea3 Mon Sep 17 00:00:00 2001 From: stockiNail Date: Wed, 6 Apr 2022 18:33:31 +0200 Subject: [PATCH 4/4] removes inner if statement because useless --- src/events.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/events.js b/src/events.js index bd47efe5b..74c1ed43d 100644 --- a/src/events.js +++ b/src/events.js @@ -25,10 +25,8 @@ export function updateListeners(chart, state, options) { if (!state.listened || !state.moveListened) { state.annotations.forEach(scope => { - if (!state.listened) { - if (typeof scope.click === 'function') { - state.listened = true; - } + if (!state.listened && typeof scope.click === 'function') { + state.listened = true; } if (!state.moveListened) { moveHooks.forEach(hook => {