Scheduler - Appointments Refactoring - Support click & double click by Tucchhaa · Pull Request #33633 · DevExpress/DevExtreme

Tucchhaa · 2026-05-19T12:17:44Z

No description provided.

Tucchhaa · 2026-05-19T12:18:50Z

    super._dispose();

-    click.off(this.$element(), EVENTS_NAMESPACE);
+    dxClick.off(this.$element(), EVENTS_NAMESPACE);


old impl also uses dxClick event

Copilot

Pull request overview

Refactors the new scheduler appointments architecture to support onAppointmentClick and onAppointmentDblClick events (previously only onAppointmentRendered was wired up), and integrates tooltip-on-click and edit-popup-on-double-click flows. Click handling is moved from ViewItem/focusController to dedicated handlers in the Appointments component, and the tooltip strategy is updated to dispatch the new onAppointmentClick event when _newAppointments is on. Tests are added covering click/dblclick callbacks, tooltip and popup flows, and recurrence dialog.

Changes:

Add onAppointmentClick/onAppointmentDblClick/onAppointmentRendered plumbing through Appointments, BaseAppointmentView, AppointmentCollector, and TooltipStrategyBase, including dxclick/dxdblclick wiring and tooltip-suppression timeout logic.
Refactor scheduler option handling: actions stored in this.actions, removing appointmentRenderedAction/createAppointmentRenderedAction; collector now consumes view models (items) instead of raw appointmentsData.
Add comprehensive Jest tests for click, double click, tooltip showing, and popup flows in new appointments.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/devextreme/js/__internal/scheduler/tooltip_strategies/tooltip_strategy_base.ts	Dispatches `onAppointmentClick` for new appointments mode on tooltip item click.
packages/devextreme/js/__internal/scheduler/m_scheduler.ts	Routes click/dblclick/rendered actions through `this.actions`; removes legacy `appointmentRenderedAction`; passes new wiring into Appointments and tooltip options.
packages/devextreme/js/__internal/scheduler/appointments/m_appointment_collection.ts	Drops unused `groups` from appointment config.
packages/devextreme/js/__internal/scheduler/appointments_new/view_item.ts	Removes built-in `onClick` from ViewItem (now handled in Appointments).
packages/devextreme/js/__internal/scheduler/appointments_new/view_item.test.ts	Updates test props to match removed `onClick`.
packages/devextreme/js/__internal/scheduler/appointments_new/appointments.ts	Adds click/dblclick handlers, tooltip-show timeout, collector click flow, and new option types.
packages/devextreme/js/__internal/scheduler/appointments_new/appointments.test.ts	Updates default props with new callbacks/showers.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment/base_appointment.ts	Switches to `dxClick`/`dxdblclick` events; exposes appointment data getters; wires `onClick`/`onDblClick` to parent.
packages/devextreme/js/__internal/scheduler/appointments_new/appointment_collector.ts	Replaces `appointmentsData` option with `items` view models; adds external `onClick`.
packages/devextreme/js/__internal/scheduler/appointments_new/mock/base_appointment_view.ts	Adds `onDblClick` mock prop.
packages/devextreme/js/__internal/scheduler/appointments_new/mock/appointment_collector.ts	Maps mocks to new `items` prop.
packages/devextreme/js/__internal/scheduler/tests/appointments_new.test.ts	New tests for click, dblclick, tooltip, popup, and recurrence dialog flows.
packages/devextreme/js/__internal/scheduler/tests/mock/model/scheduler.ts	Adds `isRecurrenceDialogVisible` helper.

Tucchhaa · 2026-05-19T12:43:00Z

+      {
+        dragBehavior: undefined, // TODO
+        isButtonClick: true,
+        tabFocusLoopEnabled: true,
+      },


old implementation also passed clickEventproperty (here).

clickEvent is used in tooltip to call onAppointmentClick handler when tooltip item is clicked.

In new implementation, onAppointmentClick is directly used in the tooltip.

P.S. when onAppointmentClick is called on the tooltip item click, setting e.cancel=true has no effect.

I think we might need to discuss it with PMs. I would suggest to make e.cancel prevent appointment popup from showing if appointment is inside the tooltip. However in this case, we would need to let user know if the clicked appointment is inside tooltip or not.

I would do that after the refactoring is done, because it's not urgent

aleksei-semikozov · 2026-05-20T10:23:47Z

+    // setTimeout is used to prevent showing tooltip on double click
+    this.appointmentClickTimeout = window.setTimeout(() => {
+      if (isElementInDom($target) && !this.preventSingleAppointmentClick) {
+        this.option().showAppointmentTooltip(
+          appointmentView.appointmentData,
+          $target,
+          appointmentView.targetedAppointmentData,
+        );
+      }
+
+      this.preventSingleAppointmentClick = false;
+    }, SHOW_TOOLTIP_TIMEOUT);


Race condition: two clicks within 300ms → both timers fire, first tooltip flashes before being redrawn for the second.

Browser repro: appointments at 8:00 and 10:00, click A then B within 300ms — onAppointmentTooltipShowing fires twice.

Failing test for appointments_new.test.ts:

it('REPRO race: no tooltip for first appointment after clicking another within 300ms', async () => { const { POM } = await createScheduler({ dataSource: [ { text: 'A', startDate: new Date(2015, 1, 9, 8), endDate: new Date(2015, 1, 9, 9) }, { text: 'B', startDate: new Date(2015, 1, 9, 10), endDate: new Date(2015, 1, 9, 11) }, ], currentView: 'day', currentDate: new Date(2015, 1, 9, 8), }); const a = POM.getAppointments().find((x) => x.getText() === 'A')!.element!; const b = POM.getAppointments().find((x) => x.getText() === 'B')!.element!; jest.useFakeTimers(); a.click(); jest.advanceTimersByTime(100); b.click(); jest.advanceTimersByTime(200); const after = POM.tooltip.isVisible() ? POM.tooltip.getAppointmentItem(0).textContent : null; jest.advanceTimersByTime(200); expect(POM.tooltip.getAppointmentItem(0).textContent).toContain('B'); expect(after).toBeNull(); // FAILS: received "A8:00 AM - 9:00 AM" });

Need to clear the previous timer before scheduling a new one.

aleksei-semikozov · 2026-05-20T10:23:47Z

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    if ((e as any).cancel) {
+      return;
+    }


If preventSingleAppointmentClick was set by a prior dblclick and the next click is cancelled, this early return skips the setTimeout that resets the flag. The next normal click then hits preventSingleAppointmentClick === true and silently skips showAppointmentTooltip.

Can't repro in browser — race above masks it (a leftover timer eventually resets the flag). Reproduces in jest:

Expected: true (tooltip should be visible after the normal click) Received: false

Once race is fixed, this surfaces. Can you double-check? Looks like cancel branch should clear pending timer + reset the flag.

aleksei-semikozov · 2026-05-20T10:23:47Z

+  private appointmentClickTimeout: number | null = null;
+
+  private preventSingleAppointmentClick = false;


Timer + flag on Appointments feel out of place. Debounce-before-tooltip is tooltip behavior, not collection state. Could discuss separately?

aleksei-semikozov · 2026-05-20T10:23:48Z

+      // @ts-expect-error
+      items: true,
+      // @ts-expect-error
+      targetedAppointmentData: true,


every @ts-expect-error — should have comments

aleksei-semikozov · 2026-05-20T10:23:48Z

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    if ((e as any).cancel) {


(e as any).cancel can be replaced with (e as Cancelable).cancel.

Cancelable is exported from @js/common/core/events:

import type { Cancelable } from '@js/common/core/events'; // ... if ((e as Cancelable).cancel) { return; }

aleksei-semikozov · 2026-05-20T10:23:48Z

+  showAppointmentTooltipCore: (
+    target: dxElementWrapper,
+    data: AppointmentTooltipItem[],
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    options?: any,
+  ) => void;


AppointmentTooltipExtraOptions is the right type here.

It's already defined in tooltip_strategies/tooltip_strategy_base.ts:70

aleksei-semikozov · 2026-05-20T11:06:30Z

+    super._setOptionsByReference();
+
+    // Note: items have appointmentData, which is used as a key in dataSource
+    this._optionsByReference = {


Why do we need the _setOptionsByReference() method?
Can you explain?

bit-byte0 · 2026-05-20T12:09:14Z

+    if (this._options.newAppointments) {
+      if (this.extraOptions?.isButtonClick) {
+        // @ts-expect-error 'component' and 'element' are set by action
+        this._options.onAppointmentClick({
+          appointmentElement: e.itemElement,
+          appointmentData: e.itemData.appointment,
+          targetedAppointmentData: e.itemData.targetedAppointment,
+          event: e.event,
+        });
+      }
+    } else {
+      this.extraOptions?.clickEvent?.(e);
+    }
+


onAppointmentClick fires here only when isButtonClick: true (i.e. tooltip opened via collector). When tooltip is opened via direct appointment click does clicking an item inside it also fire onAppointmentClick? If not, is that intentional?

implement & tests

21f38fa

Tucchhaa self-assigned this May 19, 2026

Tucchhaa requested a review from a team as a code owner May 19, 2026 12:17

Copilot AI review requested due to automatic review settings May 19, 2026 12:17

Tucchhaa added the 26_1 label May 19, 2026

Copilot started reviewing on behalf of Tucchhaa May 19, 2026 12:18 View session

Tucchhaa commented May 19, 2026

View reviewed changes

Copilot AI reviewed May 19, 2026

View reviewed changes

Tucchhaa added 2 commits May 19, 2026 20:29

fix single appt tooltip item click

2e0ab20

apply copilot's review

3183cfb

Tucchhaa commented May 19, 2026

View reviewed changes

aleksei-semikozov reviewed May 20, 2026

View reviewed changes

bit-byte0 reviewed May 20, 2026

View reviewed changes

		private appointmentClickTimeout: number \| null = null;

		private preventSingleAppointmentClick = false;

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		if ((e as any).cancel) {

Conversation

Tucchhaa commented May 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tucchhaa May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tucchhaa May 19, 2026 •

edited

Loading