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.

I noticed that the old impl has the same problem. But I have applied fix for the new impl and added a unit test

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.

fixed and added a unit test

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?

I have removed the flag, now it's only a timer. I am not sure if we need to move this logic to the tooltip, cuz it's logic may become complicated.

Right now tooltip is "dumb": it doesn't know about appointments and clicks, which I think is good

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

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


every @ts-expect-error — should have comments

added 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?

Because to access appointment in data source (e.g. to delete or update it), the scheduler uses appointmentData object reference as a key in data source.

This PR adds functionality to show appointment popup, which gets appointmentData as its parameter. When appointment popup saves data it uses the same appointmentData object to update data in the data source.

OptionManager (aka .option()) deep-copies all the options passed to the component. So in order to not lose the object reference, we need to pass appointmentData as a reference for both appointmentView and collector.

This method will be removed when usage of DOMComponent will be removed from AppointmentView and Collector

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?

Legacy implementation doesn't fire onAppointmentClick when tooltip is opened on appointment click, so to avoid any breaking changes I have made the behavior similar in the refactoring.

But in general, I think we need to rethink how onAppointmentClick works due to the reasons I have described in this comment.

Thanks for noticing, I will take it into account after the appointments refactoring is done

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

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

apply review & move some tests to unit

4277788

Copilot AI review requested due to automatic review settings May 21, 2026 12:04

Copilot started reviewing on behalf of Tucchhaa May 21, 2026 12:04 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

Comment thread packages/devextreme/js/__internal/scheduler/appointments_new/appointments.test.ts Outdated

apply copilot's review

1d06b3e

		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!

Tucchhaa May 21, 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!

Tucchhaa May 21, 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!

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!

Tucchhaa May 21, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tucchhaa May 19, 2026 •

edited

Loading

Tucchhaa May 21, 2026 •

edited

Loading

Tucchhaa May 21, 2026 •

edited

Loading

Tucchhaa May 21, 2026 •

edited

Loading