Scheduler: Appointment form customization#31555
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables propagating all dxForm options to the Scheduler's appointment form by extending the editing.form configuration to accept all dxForm properties. Previously, only a limited subset of properties (items, onSaved, onCanceled, iconsShowMode) were supported.
Key Changes
- Extended
editing.formtype definition to include alldxFormProperties/dxFormOptions - Spread user form options into the form initialization using spread operator
- Updated wrapper packages (Angular, React, Vue) to include all form properties and events
- Added test coverage to verify form options are properly propagated
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/ts/dx.all.d.ts | Updated type definition to extend dxForm.Properties instead of defining limited properties |
| packages/devextreme/js/ui/scheduler.d.ts | Changed type from inline object to FormProperties with additional scheduler-specific properties |
| packages/devextreme/js/__internal/scheduler/appointment_popup/m_form.ts | Spread userFormOptions into form initialization to propagate all dxForm options |
| packages/devextreme/js/__internal/scheduler/appointment_popup/appointment_popup.test.ts | Added test case to verify form options propagation |
| packages/devextreme-vue/src/scheduler.ts | Regenerated wrapper to include all dxForm properties, events, and imports |
| packages/devextreme-react/src/scheduler.ts | Regenerated wrapper to include all dxForm properties, events, and subscribable options |
| packages/devextreme-angular/src/ui/scheduler/nested/form.ts | Regenerated nested component with all dxForm input/output properties |
| packages/devextreme-angular/src/ui/scheduler/nested/editing.ts | Updated form type to use dxFormOptions |
| packages/devextreme-angular/src/ui/scheduler/index.ts | Updated editing property type to use dxFormOptions |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
packages/devextreme/js/__internal/scheduler/appointment_popup/m_form.ts:226
- The spread of
userFormOptionsoccurs after all internal configuration options (line 262), which means user-provided values could override critical internal options likeitems,formData,onFieldDataChanged, andonInitialized. These handlers are essential for scheduler functionality (managing recurrence form, date editor updates, resource icon colors). Consider spreadinguserFormOptionsbefore the critical internal options to prevent users from accidentally breaking scheduler behavior, or explicitly exclude certain properties from being overridden.
return this.scheduler.createComponent(element, dxForm, {
items,
formData: {},
showColonAfterLabel: false,
showValidationSummary: false,
scrollingEnabled: false,
labelLocation: 'top',
colCountByScreen: {
xs: 1,
},
elementAttr: {
class: CLASSES.form,
},
onFieldDataChanged: (e) => {
packages/devextreme-angular/src/ui/scheduler/nested/form.ts:1
- Adding two-way binding support for
formDatain the nested form component is problematic. The form'sformDatais managed internally by the scheduler and updated dynamically during appointment editing. Exposing it as a bindable property could lead to conflicts where user updates compete with internal scheduler updates, potentially causing data synchronization issues or breaking appointment editing functionality.
/* tslint:disable:max-line-length */
| ), []); | ||
|
|
||
| const subscribableOptions = useMemo(() => (["currentDate","currentView"]), []); | ||
| const subscribableOptions = useMemo(() => (["currentDate","currentView","editing.form.formData"]), []); |
There was a problem hiding this comment.
Adding 'editing.form.formData' to subscribableOptions appears incorrect. The formData property is an internal form property that gets updated dynamically as users edit appointment fields, not a scheduler option that should trigger re-renders. This could cause unnecessary re-renders or synchronization issues. The scheduler manages form data internally through createForm, and users shouldn't be subscribing to this nested path.
| const subscribableOptions = useMemo(() => (["currentDate","currentView","editing.form.formData"]), []); | |
| const subscribableOptions = useMemo(() => (["currentDate","currentView"]), []); |
aleksei-semikozov
force-pushed
the
form-options
branch
from
ad71604 to
0a892b0
Compare
aleksei-semikozov
force-pushed
the
form-options
branch
2 times, most recently
from
1862952 to
61488df
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/devextreme/js/__internal/scheduler/appointment_popup/m_form.ts:263
- Deep merge with
extend(true, ...)will merge theonInitializedandonFieldDataChangedcallbacks fromdefaultOptionsandcustomFormOptions, potentially causing issues. If a user provides these callbacks incustomFormOptions, both callbacks would be merged (object merge), not executed in sequence. You should handle these lifecycle callbacks specially - either by wrapping them to call both, or by ensuring custom callbacks take precedence while preserving the default behavior. For example, save references to default handlers and conditionally chain them.
return;
packages/devextreme/js/__internal/scheduler/appointment_popup/m_form.ts:211
- When
editingConfigis a boolean (e.g.,editing: true),editingConfig.formwill be undefined and default to{}. However, consider checkingisBoolean(editingConfig)here for clarity and consistency with the existinggetIconsShowMode()method pattern on line 201.
const recurrenceGroup = this._recurrenceForm.createRecurrenceFormGroup();
a48fc35 to
2915b01
Compare
…appointment_popup.test.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Aleksei Semikozov <alwexy@gmail.com>
aleksei-semikozov
force-pushed
the
form-options
branch
from
2915b01 to
6e90af8
Compare
aleksei-semikozov
force-pushed
the
form-options
branch
from
921a301 to
4c7b31d
Compare
aleksei-semikozov
force-pushed
the
form-options
branch
from
4c7b31d to
342b784
Compare
aleksei-semikozov
force-pushed
the
form-options
branch
from
342b784 to
48b7bf0
Compare
aleksei-semikozov
force-pushed
the
form-options
branch
from
48b7bf0 to
9dcc3f5
Compare
| const defaultOptions: FormProperties = { | ||
| items, |
There was a problem hiding this comment.
The items parameter is used for default options, but formItems from custom config is destructured and ignored. This could lead to confusion about which items are actually used. Consider documenting this behavior or renaming to clarify that custom items should be part of customFormOptions.
| } as FormProperties) as dxForm; | ||
| } as FormProperties; | ||
|
|
||
| const formOptions = extend(true, defaultOptions, customFormOptions); |
There was a problem hiding this comment.
[nitpick] Add a comment explaining that deep merge is intentional here to allow partial overrides of nested options like elementAttr while preserving default values for non-specified properties.
aleksei-semikozov
force-pushed
the
form-options
branch
from
9dcc3f5 to
4758fab
Compare
| allowTimeZoneEditing: Boolean, | ||
| allowUpdating: Boolean, | ||
| form: Object as PropType<Record<string, any>>, | ||
| form: {}, |
There was a problem hiding this comment.
The type of the form prop has been changed from a specific typed object to an empty object literal {}. This loses type safety and makes it harder for developers to discover what properties are available. Consider using Object as PropType<Record<string, any>> to maintain consistency with other similar props in the file (like line 403 for popup), or better yet, importing and using the proper SchedulerAppointmentFormOptions type.
| form: {}, | |
| form: Object as PropType<Record<string, any>>, |
| onCanceled?: ((formData: any) => void); | ||
| onSaved?: ((formData: any) => void); | ||
| }; | ||
| form?: any; |
There was a problem hiding this comment.
The type of the form property has been changed to any, which eliminates type safety. This is inconsistent with the approach in the TypeScript definitions where a proper SchedulerAppointmentFormOptions type was created. Consider importing and using the proper type from devextreme/ui/scheduler to maintain type safety and IntelliSense support for developers.
| get form(): any { | ||
| return this._getOption('form'); | ||
| } | ||
| set form(value: undefined | { iconsShowMode?: AppointmentFormIconsShowMode, items?: Array<dxFormButtonItem | dxFormEmptyItem | dxFormGroupItem | dxFormSimpleItem | dxFormTabbedItem>, onCanceled?: ((formData: any) => void), onSaved?: ((formData: any) => void) }) { | ||
| set form(value: any) { |
There was a problem hiding this comment.
The form property type has been changed from a specific typed object to any, losing type safety. Similar to the React and Vue wrappers, this should use the proper SchedulerAppointmentFormOptions type from devextreme/ui/scheduler to maintain type safety and developer experience consistency across all framework wrappers.
No description provided.