DataGrid: Refactor ResizingController._synchronizeColumns - #34325
DataGrid: Refactor ResizingController._synchronizeColumns#34325nightskylark wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a DataGrid sizing/resizing issue where column width updates may not be applied immediately by refactoring parts of the grid’s column synchronization flow (best-fit toggling, max-width handling) and tightening selection-range handling used during temporary layout measurement. It also includes a small TypeScript-typing workaround in the Popover escape-key handler.
Changes:
- Refactors
ResizingController._synchronizeColumnsto use a temporary best-fit enable/restore helper, normalize group expand column widths, and centralize max-width set/clear logic. - Introduces a typed
SelectionRangecontract and updates selection-range getters/setters to use explicit sentinel values. - Adjusts Popover overlay-stack comparison typing to avoid a TypeScript “no overlap” error.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/devextreme/js/__internal/ui/popover/popover.ts | Tweaks overlay stack top-check typing in ESC key handler. |
| packages/devextreme/js/__internal/grids/grid_core/views/m_grid_view.ts | Refactors grid column synchronization/best-fit/maxWidth handling related to resizing. |
| packages/devextreme/js/__internal/grids/grid_core/m_utils.ts | Adds SelectionRange type and makes selection-range APIs more explicit/typed. |
| } | ||
| return undefined; | ||
| }); | ||
| private _synchronizeColumns():void { |
… and related logic
| return freeWidth / columnCountWithoutWidth; | ||
| } | ||
|
|
||
| private readonly _normalizeWidthsByExpandColumns = (resultWidths, visibleColumns): void => { |
There was a problem hiding this comment.
It's not clear why this method is marked as readonly. In this case, we're explicitly preventing it from being overridden without any clear reason. That goes against the Grid's architecture, which is built around inheritance and @Extended overrides.
Also, the method parameters are untyped.
| } | ||
|
|
||
| private readonly _normalizeWidthsByExpandColumns = (resultWidths, visibleColumns): void => { | ||
| const expandColumnIndex = visibleColumns.findIndex((column) => column.type === 'groupExpand'); |
There was a problem hiding this comment.
Now the Grid uses the width of the first expand column it encounters. Previously, it used the width of the last expand column.
| const expandColumnIndex = visibleColumns.findIndex((column) => column.type === 'groupExpand'); | ||
| const expandColumnWidth = resultWidths[expandColumnIndex]; | ||
|
|
||
| if (!isDefined(expandColumnWidth)) { |
There was a problem hiding this comment.
We no longer ignore the value 0. Is this intentional?
|
|
||
| public resizeCompleted!: Callback; | ||
|
|
||
| private readonly _maxWidth: MaxWidthController = { |
There was a problem hiding this comment.
This approach is a bit unusual for the Grid. Having two regular private methods, _setMaxWidth and _clearMaxWidth, along with a boolean flag, would be more consistent with the existing codebase.
| private readonly _maxWidth: MaxWidthController = { | ||
| isModified: false, | ||
| set: (value): void => { | ||
| const $element = this.component.$element(); |
There was a problem hiding this comment.
Shouldn't we also check whether $element exists here, just like we do in the clear method?
nightskylark
changed the title
ResizingController._synchronizeColumnsandm_utils.tswere refactored for readability and type-safety ahead of the fix, without changing behavior:ResizingController:_maxWidth: anyfield with a small_maxWidth: MaxWidthControllerhelper object (isModified,set(),clear()) that encapsulates reading/writing the element'smaxWidthCSS, instead of manually checking truthiness and mutating the DOM inline in_synchronizeColumns._synchronizeColumnsinto a new_enableTemporaryBestFitMode()method that returns a cleanup closure, replacing the previousresetBestFitModeboolean flag plus an inline restore-focus block.normalizeWidthsByExpandColumnsclosure into a proper private method_normalizeWidthsByExpandColumns(resultWidths, visibleColumns), simplified withfindIndexinstead of two separateeachloops.needBestFit/hasMinWidthcomputation from imperative loops with early-exit flags into single.some()expressions, and narrowedresultWidthsto(number | string | undefined)[], scoped inside thedeferUpdatecallback instead of the outer closure.This refactor made
_synchronizeColumnseasier to reason about and was a prerequisite for isolating and fixing thevisibleWidth/widthstaleness bug (T1329677) in the same best-fit measurement code path.