Widgets: Remove code related width, height option as function (FilterBuilder, DateBox, DropDownEditor, DropDownButton, Lookup, Menu, Toolbar.Menu)#33177
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes usage of function-valued width/height/maxHeight options across several widgets (Menu, Toolbar menu, FilterBuilder, DropDownEditor family, Lookup, DateBox), updating runtime code and adjusting tests to assert numeric option values instead of invoking option functions.
Changes:
- Replace function-valued popup/overlay dimension options with numeric values and add resize/dimension-change updates where needed.
- Update QUnit tests to treat
maxHeight/dropDownOptions.widthas values (not callables). - Add targeted widget-specific handling (e.g., DateBox calendar strategies, toolbar menu maxHeight updates).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets/menu.tests.js | Updates Menu adaptivity test to treat maxHeight as a value. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/filterBuilderParts/commonTests.js | Updates FilterBuilder-related test to treat maxHeight as a value. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/lookup.tests.js | Updates Lookup test to treat dropDownOptions.width as a value. |
| packages/devextreme/js/__internal/ui/toolbar/internal/toolbar.menu.ts | Makes toolbar menu popup maxHeight numeric and refreshes it on _dimensionChanged. |
| packages/devextreme/js/__internal/ui/menu/menu.ts | Makes adaptive overlay maxHeight numeric and refreshes it on _dimensionChanged. |
| packages/devextreme/js/__internal/ui/m_lookup.ts | Adjusts Lookup dropdown sizing logic (some widths now numeric; some remain function-based). |
| packages/devextreme/js/__internal/ui/m_drop_down_button.ts | Makes DropDownButton popup width numeric and updates width-setting paths accordingly. |
| packages/devextreme/js/__internal/ui/m_drop_down_box.ts | Adds clarification comment around function-valued maxHeight behavior. |
| packages/devextreme/js/__internal/ui/drop_down_editor/m_drop_down_editor.ts | Makes popup width numeric and changes popup-width update logic. |
| packages/devextreme/js/__internal/ui/date_box/date_box.base.ts | Prevents base popup-width auto-updates for Calendar strategies (which set popup width to auto). |
| packages/devextreme/js/__internal/scheduler/appointment_popup/appointment_popup.test.ts | Adds a @ts-expect-error to silence a callback signature mismatch in a test. |
| packages/devextreme/js/__internal/scheduler/tests/mock/create_appointment_popup.ts | Adds a @ts-expect-error to silence popup.show(...) typing mismatch in test mock. |
| packages/devextreme/js/__internal/filter_builder/m_filter_builder.ts | Makes FilterBuilder popup maxHeight numeric. |
| _updatePopupWidth(): void { | ||
| const popupWidth = getSizeValue(this.option('dropDownOptions.width')); | ||
| const popupWidth = getSizeValue(this.option('_cached_dropDownOptions.width')); | ||
|
|
||
| if (popupWidth === undefined) { | ||
| this._setPopupOption('width', () => getElementWidth(this.$element())); | ||
| this._setPopupOption('width', getElementWidth(this.$element())); | ||
| } |
There was a problem hiding this comment.
_updatePopupWidth() now reads this.option('_cached_dropDownOptions.width'), but _cached_dropDownOptions is only initialized once in _init() and is not refreshed when dropDownOptions is replaced via option('dropDownOptions', ...). This can cause user-specified dropDownOptions.width to be ignored and overwritten with the element width. Consider reading dropDownOptions.width directly here, or updating the cache whenever dropDownOptions changes (including full-object replacement).
| dropDownOptions: { | ||
| showTitle: true, | ||
| // @ts-expect-error ts-error | ||
| width() { | ||
| return getSize('width'); | ||
| }, | ||
| // @ts-expect-error ts-error | ||
| height() { | ||
| return getSize('height'); | ||
| }, | ||
| // @ts-expect-error Category 3 — There is no built-in mechanism for mandatory updates | ||
| width: () => getSize('width'), | ||
| // @ts-expect-error Category 3 — There is no built-in mechanism for mandatory updates | ||
| height: () => getSize('height'), |
There was a problem hiding this comment.
The PR title indicates removing function-based width/height options for Lookup, but dropDownOptions.width/height are still set to functions here and type errors are suppressed. If function values are no longer supported, this can break popup sizing at runtime; instead, compute numeric values and update them on relevant resize/viewport-change events (e.g., via _dimensionChanged + visualViewport handlers).
| dropDownOptions: { | ||
| _ignoreFunctionValueDeprecation: true, | ||
|
|
||
| width: () => getElementWidth(this.$element()), | ||
| width: getElementWidth(this.$element()), | ||
| height: function () { return this._getPopupHeight(); }.bind(this), |
There was a problem hiding this comment.
In the Material defaultOptionsRule, dropDownOptions.width is now computed immediately via getElementWidth(this.$element()). If the widget is initialized while hidden or before final layout, this value can be 0/stale and will prevent the base popup-width auto-update logic from running (since width becomes defined). Consider leaving dropDownOptions.width undefined and relying on _updatePopupWidth(), or recomputing it when the component becomes visible/dimensions change.
| // The overlay calls maxHeight every time it calculates the content dimensions, | ||
| // this._popupPosition?.v.location, which is updated after each repositioning |
There was a problem hiding this comment.
The new comment is grammatically incomplete (second line is a sentence fragment) and is hard to understand as-is. Please rephrase into a complete explanation of why maxHeight must remain a function here (e.g., tie it explicitly to repositioning and _popupPosition.v.location).
| // The overlay calls maxHeight every time it calculates the content dimensions, | |
| // this._popupPosition?.v.location, which is updated after each repositioning | |
| // maxHeight must remain a function because the overlay reevaluates it whenever | |
| // it recalculates content dimensions, and it depends on this._popupPosition?.v.location, | |
| // which is updated after each repositioning. |
| const onSave = jest.fn((newAppointment) => { | ||
| updateAppointment(sourceAppointment, updatedAppointment); | ||
| // @ts-expect-error Expected 0 arguments, but got 1 | ||
| return addAppointment(newAppointment); |
There was a problem hiding this comment.
Adding @ts-expect-error here hides a real signature mismatch in the test (the mock addAppointment is typed as taking 0 args, but the scenario calls it with the new appointment). It would be safer to adjust the mock/callback typings so addAppointment(appointment) is valid, rather than suppressing the error.
| // @ts-expect-error onSave | ||
| popup.show(appointmentData, { onSave, title, readOnly }); |
There was a problem hiding this comment.
@ts-expect-error on popup.show(...) makes the mock less type-safe and can mask real API drift. Prefer updating the AppointmentPopup.show typing (or the mock wrapper types) so the { onSave, title, readOnly } argument is accepted without suppression, matching how appointmentPopup.show is used in scheduler code.
No description provided.