Repository navigation
Grids: type m_grid_view - #35598
Grids: type m_grid_view#35598anna-shakhova wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The master-detail resize callback can call .done() on an undefined result when the detail grid becomes invisible.
1 open finding
What changed in this PR
Adds stronger TypeScript typing to grid view and resizing logic while preserving existing behavior.
Changes:
- Types grid view controllers, methods, deferreds, and view lookups.
- Replaces legacy iterator patterns with typed array operations.
- Updates master-detail handling for the typed
updateDimensionsresult.
| File | Description |
|---|---|
packages/devextreme/js/__internal/grids/grid_core/views/m_grid_view.ts |
Adds types and modernizes grid sizing/view logic. |
packages/devextreme/js/__internal/grids/grid_core/master_detail/m_master_detail.ts |
Adapts master-detail resizing to the new return type. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
c7bdb72 to
8028ad6
Compare
| ): ModuleType<ResizingController> => class AdaptivityResizingViewControllerExtender extends Base { | ||
| public dispose() { | ||
| super.dispose.apply(this, arguments as any); | ||
| clearTimeout(this._updateScrollableTimeoutID); |
There was a problem hiding this comment.
_updateScrollableTimeoutID was never set, so remove unnecessary method overload
| // @ts-expect-error columns without a width have best-fit widths here | ||
| resultSizes[i] += diffElement; | ||
| if (onePixelElementsCount > 0) { | ||
| if (onePixelElementsCount < 1) { | ||
| resultSizes[i] += onePixelElementsCount; | ||
| onePixelElementsCount = 0; | ||
| } else { | ||
| resultSizes[i]++; | ||
| onePixelElementsCount--; | ||
| } | ||
| if (onePixelElementsCount > 0 && onePixelElementsCount < 1) { | ||
| // @ts-expect-error columns without a width have best-fit widths here | ||
| resultSizes[i] += onePixelElementsCount; |
There was a problem hiding this comment.
the += 1 branch already became Number(resultSizes[i]) + 1. Could the two += lines take the same form, so both @ts-expect-error go away? The values here are numbers either way
| // @ts-expect-error columns without a width have best-fit widths here | |
| resultSizes[i] += diffElement; | |
| if (onePixelElementsCount > 0) { | |
| if (onePixelElementsCount < 1) { | |
| resultSizes[i] += onePixelElementsCount; | |
| onePixelElementsCount = 0; | |
| } else { | |
| resultSizes[i]++; | |
| onePixelElementsCount--; | |
| } | |
| if (onePixelElementsCount > 0 && onePixelElementsCount < 1) { | |
| // @ts-expect-error columns without a width have best-fit widths here | |
| resultSizes[i] += onePixelElementsCount; | |
| resultSizes[i] = Number(resultSizes[i]) + diffElement; | |
| if (onePixelElementsCount > 0 && onePixelElementsCount < 1) { | |
| resultSizes[i] = Number(resultSizes[i]) + onePixelElementsCount; |
| } | ||
|
|
||
| const createResizingController = (): ResizingControllerInternals => { | ||
| const controller = Object.create(ResizingController.prototype) as ResizingControllerInternals; |
There was a problem hiding this comment.
Object.create skips the constructor and init(), so these tests may break if the methods start using another field. Could we move this width math into pure functions and test them directly?
| // @ts-expect-error updateDimensions() returns undefined only for an invisible grid | ||
| // eslint-disable-next-line @typescript-eslint/no-misused-promises -- ignores the result | ||
| this.updateDimensions().done(() => d.resolve(true)); |
There was a problem hiding this comment.
updateDimensions() returns undefined for an invisible grid, so .done would throw here. What do you think about when(...) like in resize(), instead of @ts-expect-error?
| // @ts-expect-error updateDimensions() returns undefined only for an invisible grid | |
| // eslint-disable-next-line @typescript-eslint/no-misused-promises -- ignores the result | |
| this.updateDimensions().done(() => d.resolve(true)); | |
| when(this.updateDimensions()).done(() => { | |
| d.resolve(true); | |
| }); |
No description provided.