Scheduler - Refactor Appointments Collection - KBN by bit-byte0 · Pull Request #33643 · DevExpress/DevExtreme

bit-byte0 · 2026-05-20T08:40:59Z

What

Keyboard navigation for new appointments_new/ architecture (Del/Home/End/Enter/Space)

How

New props: editing.allowDeleting, onDeleteKeyPress, onItemActivate on AppointmentsProperties
focus_controller: switch(true) per key in onViewItemKeyDown
Del passes correct occurrence startDate, recurring delete excludes right instance

…ents tests

Copilot

Pull request overview

This PR extends the new Scheduler appointments architecture (appointments_new/) with keyboard navigation/activation and delete-by-key support, wiring these behaviors up from Scheduler into the new Appointments component and adding Jest coverage for the new key interactions.

Changes:

Added allowDelete, onDeleteKeyPress, and onItemActivate to AppointmentsProperties and wired them from Scheduler.
Expanded AppointmentsFocusController key handling to support Delete/Home/End/Enter/Space.
Updated/added Jest tests to use @testing-library/dom key events and to cover the new keyboard behaviors.

Reviewed changes

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

Show a summary per file

File	Description
packages/devextreme/js/__internal/scheduler/m_scheduler.ts	Passes delete/activate callbacks + `allowDelete` into new `Appointments` configuration.
packages/devextreme/js/__internal/scheduler/appointments_new/appointments.ts	Extends `AppointmentsProperties` and provides default options for new callbacks/flags.
packages/devextreme/js/__internal/scheduler/appointments_new/appointments.focus_controller.ts	Implements the new key handling logic (Delete/Home/End/Enter/Space).
packages/devextreme/js/__internal/scheduler/appointments_new/appointments.test.ts	Updates keydown simulation and adds unit tests for Home/End and keyboard actions.
packages/devextreme/js/__internal/scheduler/tests/appointments_new.test.ts	Adds integration tests for deleting appointments via the Delete key (including recurring occurrences).

Tucchhaa · 2026-05-21T07:54:12Z

      const appointmentsConfig: Partial<AppointmentsProperties> = {
        tabIndex: this.option('tabIndex'),
        currentView: this.option('currentView') as ViewType,
+        allowDelete: this.editing.allowUpdating && this.editing.allowDeleting,


allowDelete value is only passed to Appointments on init, but there wasn't added a code that would update allowDelete on runtime change.

But actually deleting works as expected, because the legacy implementation does it for new appointments here.

Even though it works, I think we can improve something there:

this repaint call is redundant for the new appointments.

Weird calculation for allowDelete. In tooltip to delete appointmen just editing.allowDelete is used.

What I suggest to do:

pass scheduler.editing.allowDelete value to Appointments. Scheduler normalizes the 'editing' option here. We need to pass this to new Appointments

Don't call bringEditingModeToAppointments here for new Appointments. Just update Appointments.option().allowDeleting directly via: Appointments.option('allowDeleting' this.editing.allowDeleting)

Add integration tests for these cases (here):

a. Appt can be deleted when editing=true
b. Appt can be deleted when editing.allowDeleting=true
c. Appt can be deleted when editing.allowDeleting=true and editing.allowUpdating=false
d. Appt cant be deleted when editing.allowDeleting=false
e. Appt cant be deleted when editing=false

I think it can be a matrix test.

Please let me know if you have any questions

In case c (allowDeleting=true, allowUpdating=false) - should Delete key work? The tooltip only checks allowDeleting, but allowUpdating=false implies the data is read-only. Should keyboard delete follow the same logic as tooltip, or should it respect allowUpdating too?

I think we should follow tooltip's behavior (only check allowDeleting), because doc doesn't state that allowUpdating must be enabled too to allow appt deletion

Tucchhaa · 2026-05-21T08:02:58Z

+    const { allowDelete, onDeleteKeyPress, getDataAccessor } = this.appointments.option();
+    if (!allowDelete) { return; }
+
+    const entity = this.sortedAppointments[sortedIndex];


Let's avoid variable name entity because it's very generic and tells nothing.

I see that the name of the type is SortedEntity, but it's also something that we can improve.

I suggest to rename entity to itemData, the same applies to all other places where name entity is used

SortedEntity already has a field called .itemData (the appointment data inside). So if we name the variable itemData, you'd write itemData.itemData to access the appointment.

How about sortedItem instead - consistent with sortedIndex, sortedAppointments, and sortedItems used elsewhere?

You're right, I agree with sortedItem. Can you please update names of other occurrences of itemData?
focusByItemData -> focusBySortedItem

What do you think about renaming getSortedAppointments to getSortedItems, so that it would be consistent everywhere?

Let's try it out

Tucchhaa · 2026-05-21T08:08:47Z

+    if (!allowDelete) { return; }
+
+    const entity = this.sortedAppointments[sortedIndex];
+    if (!entity) { return; }


If 'delete' was pressed on a collector item, e.g.:

Then an exception is thrown in the console.

Old appointments implementation also throws some kind of error. I think we might need to discuss it in a call. But I would suggest to prevent appointment deleting in this case

Confirmed - the guard doesn't protect here because collector sortedIndex may overlap with sortedAppointments indices, potentially deleting the wrong appointment. We'll add an explicit instanceof AppointmentCollector check in onViewItemKeyDown to skip Delete on collectors entirely. This matches your suggestion to prevent deleting in this case. What do you think?

Yep, that's ok for me. Please also add a test.

And small suggestion: let's move this if-statement inside the handleDeleteKeydown and pass viewItem to all the keydown handlers instead of sortedIndex

Tucchhaa · 2026-05-21T08:13:19Z

+    const occurrence = { ...entity.itemData };
+    getDataAccessor().set('startDate', occurrence, new Date(entity.source.startDate));
+
+    onDeleteKeyPress({ appointment: entity.itemData, occurrence });


This is a strange way to get occurrence data, because there's a dedicated function for that: getTargetedAppointmentData.

I suggest to make BaseAppointment.targetedAppointmentData and BaseAppointment.appointmentData properties to be public, so we could use it here:

onDeleteKeyPress({ appointmentData: viewItem.appointmentData, targetedAppointmentData: viewItem.targetedAppointmentData })

Actually I have made these props in my PR to be public

Tucchhaa · 2026-05-21T08:24:08Z

+
+  private handleHomeKeyDown(): void {
+    const firstAppointment = this.sortedAppointments[0];
+    if (firstAppointment) { this.focusByItemData(firstAppointment); }


We need e.preventDefault here, like in the legacy impl.

Otherwise body element may scroll to the bottom if it has large height.

Please add a jest test to check that event is prevented on 'home' press

Tucchhaa · 2026-05-21T08:24:37Z

+
+  private handleEndKeyDown(): void {
+    const lastAppointment = this.sortedAppointments[this.sortedAppointments.length - 1];
+    if (lastAppointment) { this.focusByItemData(lastAppointment); }


Same as for Home key down: https://github.com/DevExpress/DevExtreme/pull/33643/changes#r3279707087

Copilot

Pull request overview

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

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

bit-byte0 added 4 commits May 18, 2026 19:15

refactor(scheduler): appointments KBN

c79f4f8

feat(scheduler): add Delete key support for appointments_new KBN

e8de742

feat(scheduler): add KBN home/end/enter/space + fix delete occurrence

9b38cbf

refactor(scheduler): replace dispatchEvent with fireEvent in appointm…

7c91113

…ents tests

bit-byte0 self-assigned this May 20, 2026

bit-byte0 requested a review from a team as a code owner May 20, 2026 08:41

Copilot AI review requested due to automatic review settings May 20, 2026 08:41

bit-byte0 added the 26_1 label May 20, 2026

Copilot started reviewing on behalf of bit-byte0 May 20, 2026 08:41 View session

Copilot AI reviewed May 20, 2026

View reviewed changes

fix(scheduler): use week view in recurring delete integration test

d1f3a12

Tucchhaa reviewed May 21, 2026

View reviewed changes

Tucchhaa requested changes May 21, 2026

View reviewed changes

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

Copilot started reviewing on behalf of bit-byte0 May 21, 2026 10:12 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings May 21, 2026 11:58

Copilot started reviewing on behalf of bit-byte0 May 21, 2026 11:59 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

refactor(scheduler): address KBN review feedback for appointments_new

9c60beb

bit-byte0 force-pushed the refactor/scheduler-appointments-kbn branch from c517882 to 9c60beb Compare May 21, 2026 22:03

Conversation

bit-byte0 commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

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!

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!

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!

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!

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bit-byte0 commented May 20, 2026 •

edited

Loading

Tucchhaa May 21, 2026 •

edited

Loading

Tucchhaa May 21, 2026 •

edited

Loading

Tucchhaa May 21, 2026 •

edited

Loading

Tucchhaa May 21, 2026 •

edited

Loading