Scheduler - Appointments Refactoring - Rendering#33014
Scheduler - Appointments Refactoring - Rendering#33014Tucchhaa merged 16 commits intoDevExpress:26_1from
Conversation
| startDate: Date | string; | ||
| endDate: Date | string; |
There was a problem hiding this comment.
SafeAppointment type actually doesn't have this properties, so I have removed them
There was a problem hiding this comment.
Then we should remove SafeAppointment? Now there is no point in having it
There was a problem hiding this comment.
Pull request overview
This PR introduces a new internal Scheduler appointments rendering implementation (behind the _newAppointments option), including incremental rendering via view model diffing, plus supporting utilities, styling updates, and test coverage.
Changes:
- Added
appointments_newcomponents (appointments container, base appointment, grid/agenda appointments, collector) and utilities (diffing, targeted appointment, date text) with Jest tests. - Integrated the new appointments implementation into
m_scheduler.tswhen_newAppointmentsis enabled. - Updated Scheduler appointment CSS selectors and refreshed TestCafe visual etalons accordingly.
Reviewed changes
Copilot reviewed 27 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/__internal/scheduler/utils/get_targeted_appointment.ts | Marks old targeted-appointment helper for removal after legacy deletion. |
| packages/devextreme/js/__internal/scheduler/types.ts | Simplifies SafeAppointment typing to align with Scheduler Appointment (Date | string dates already supported). |
| packages/devextreme/js/__internal/scheduler/m_subscribes.ts | Switches legacy date-text formatting to new helper and adds legacy-compat TODOs. |
| packages/devextreme/js/__internal/scheduler/m_scheduler.ts | Wires in new Appointments component behind _newAppointments, including onAppointmentRendered action bridging and container wiring. |
| packages/devextreme/js/__internal/scheduler/m_compact_appointments_helper.ts | Marks legacy helper for removal. |
| packages/devextreme/js/__internal/scheduler/m_classes.ts | Marks legacy class exports for cleanup after old impl removal. |
| packages/devextreme/js/__internal/scheduler/appointments_new/utils/type_helpers.ts | Adds type guards for new appointment view model variants. |
| packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_view_model_diff.ts | Adds diffing logic supporting add/remove/resize operations for incremental rendering. |
| packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_view_model_diff.test.ts | Adds Jest coverage for diffing behavior. |
| packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_targeted_appointment.ts | Adds targeted-appointment construction for new appointment view models (incl. resources). |
| packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_targeted_appointment.test.ts | Adds Jest coverage for targeted appointment construction and resource enrichment. |
| packages/devextreme/js/__internal/scheduler/appointments_new/utils/get_date_text.ts | Adds date text formatting utilities used by new/bridged rendering paths. |
| packages/devextreme/js/__internal/scheduler/appointments_new/const.ts | Introduces shared CSS class constants and localized strings for new appointments rendering. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointments.ts | New appointments container component implementing diff-based incremental DOM updates. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointments.test.ts | Jest coverage for rendering, partial updates, all-day container routing, and onAppointmentRendered. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment_collector.ts | New appointment-collector component (button-based) for compact/“more” UI. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment_collector.test.ts | Jest coverage for collector CSS classes, aria, positioning, and text. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment/grid_appointment.ts | New grid appointment component (geometry + content + resource color). |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment/grid_appointment.test.ts | Jest coverage for grid appointment rendering details and resource coloring. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment/base_appointment.ts | Base appointment component with templating, action dispatch, aria role, and common helpers. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment/base_appointment.test.ts | Jest coverage for base appointment classes and basic aria role. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment/agenda_appointment.ts | New agenda appointment component with marker, details, and resource list rendering. |
| packages/devextreme/js/__internal/scheduler/appointments_new/appointment/agenda_appointment.test.ts | Jest coverage for agenda appointment behavior incl. resources and recurrence. |
| packages/devextreme/js/__internal/scheduler/appointments_new/mock/appointment_properties.ts | Test helpers for generating mock view models and base appointment props. |
| packages/devextreme-scss/scss/widgets/material/scheduler/_index.scss | Updates selectors to apply appointment-content styling without relying on .dx-item-content. |
| packages/devextreme-scss/scss/widgets/generic/scheduler/_index.scss | Same selector adjustment for generic theme. |
| packages/devextreme-scss/scss/widgets/fluent/scheduler/_index.scss | Same selector adjustment for fluent theme. |
| e2e/testcafe-devextreme/tests/scheduler/common/layout/adaptive/etalons/view=day-crossScrolling=true-vertical-rtl (fluent.blue.light).png | Updated visual baseline due to styling/rendering changes. |
| e2e/testcafe-devextreme/tests/scheduler/common/layout/adaptive/etalons/view=day-crossScrolling=true-rtl (fluent.blue.light).png | Updated visual baseline due to styling/rendering changes. |
| e2e/testcafe-devextreme/tests/scheduler/common/layout/adaptive/etalons/view=day-crossScrolling=false-vertical-rtl (fluent.blue.light).png | Updated visual baseline due to styling/rendering changes. |
|
|
||
| export class BaseAppointment< | ||
| TProperties extends BaseAppointmentProperties = BaseAppointmentProperties, | ||
| > extends DOMComponent<BaseAppointment<TProperties>, TProperties> { |
There was a problem hiding this comment.
I looked at what DOMComponent features are actually used here:
$element()— just a jQuery refoption()— just a props object_getTemplateByOption()— one call:template.render({ container, model }). Parent can do this._createActionByOption()— could be a direct callback
The only place that really needs DOMComponent is _createComponent in AppointmentCollector — it creates a Button and handles disabled/rtl cascading.
But that does not mean every appointment needs to be a DOMComponent. The Collector can get a button factory from the parent instead.
Also: focus() is an empty stub. We lost KBN and accessibility that CollectionWidget gave us for free.
I think each appointment should be a simple object (jQuery element + position + classes), not a full DOMComponent. Template rendering should stay in the parent — like how CollectionWidget does it for List, Gallery, TileView.
There was a problem hiding this comment.
Very good point, but I would suggest to postpone removal of DOMComponent. But right now it's a familiar mechanism to pass properties and use common functions.
DOMComponent doesn't introduce a lot of overheads to the appointments and I believe it will be easy to remove DOMComponent in the future.
If you don't mind I would postpone this
|
Good work on isolating the appointment components and the diff-based rendering idea. I left inline comments, but here is the summary: 1. Feature flag: We are adding ~2800 lines behind I think we can do this step by step — replace old code with new code directly, no flag needed:
Each step is small, tested in production, and removes old code instead of adding dead code. 2. No integration tests: 77 unit tests, but no test with a real Scheduler. We do not know if this works end-to-end. 3. DOMComponent per appointment: Each appointment is a full DOMComponent — with template manager, action system, options, lifecycle. But it is just a rectangle with text. This is too much. If we moved away from CollectionWidget, we should use something simpler, not something equally heavy. See inline comments. Let's discuss the approach before merging. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 34 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/devextreme/js/__internal/scheduler/m_scheduler.ts:998
createAppointmentRenderedAction()creates theonAppointmentRenderedaction withoutexcludeValidators: ['disabled', 'readOnly'], while the legacy path uses that exclusion ingetAppointmentRenderedAction(). With_newAppointmentsenabled, this changes when the handler runs (e.g., it may stop firing when the scheduler is disabled/readOnly). To preserve consistent public event behavior across implementations, pass the sameexcludeValidatorsoptions when creatingappointmentRenderedAction.
| startDate: Date | string; | ||
| endDate: Date | string; |
There was a problem hiding this comment.
Then we should remove SafeAppointment? Now there is no point in having it
| return { | ||
| text: adapter.text || messageLocalization.format('dxScheduler-noSubject'), | ||
| formatDate: formatDates(startDate, endDate, formatType), | ||
| formatDate: getDateText(startDate, endDate, formatType as any), |
There was a problem hiding this comment.
If you specify "format" argument in createFormattedDateText as of type DateFormatType, then we don't need to cast formatType as any.
| } | ||
| case diffItem.needToAdd: { | ||
| const fragment = allDay ? allDayFragment : commonFragment; | ||
| const appointment = this.renderAppointment(fragment, diffItem.item); |
There was a problem hiding this comment.
This will break for collectors.
When a collector aggregates all-day appointments, it gets allDay: true from the view model. So it goes into allDayFragment → $allDayContainer. But collectors are compact overflow buttons with absolute positioning — they should always go into $commonContainer.
const fragment = (allDay && !isAppointmentCollectorViewModel(diffItem.item))
? allDayFragment : commonFragment;There was a problem hiding this comment.
Ok, thank you. I will fix this in the future. I am sure, that not all of the cases are covered in this PR, so I expect that we will notice more and more such cases and fix them pointwise
No description provided.