Repository navigation
GridCore - EditorFactory & Adaptivity - fix eslint & ts errors - #35703
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are focused, internally consistent, and preserve existing runtime behavior.
0 open findings
What changed in this PR
Improves GridCore editor factory type safety and resolves lint/TypeScript issues without changing intended behavior.
Changes:
- Adds explicit types and modern null-safe syntax.
- Documents focus-overlay initialization guarantees.
- Cleans up focus and event-handler logic.
| File | Description |
|---|---|
m_editor_factory.ts |
Strengthens editor factory typing and simplifies focus handling. |
m_sticky_columns.ts |
Handles focus-overlay type checks in sticky-column extensions. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| function getColumnId(that, column) { | ||
| return that._columnsController.getColumnId(column); | ||
| function getColumnId( |
There was a problem hiding this comment.
Confusing utility, it should be method of columnsController, but according to utility body it is already a method. Let's use method inline instead of this utility
There was a problem hiding this comment.
| export class AdaptiveColumnsViewController extends modules.ViewController { |
| cellOptions: AdaptiveDetailCellOptions, | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| updateForm?: (hiddenColumns?: Column[]) => void, | ||
| ): (options: unknown, container: DxElement) => void { |
There was a problem hiding this comment.
is it possible to type options from usage?
| private _isVisibleColumnsValid(visibleColumns) { | ||
| const getCommandColumnsCount = function () { | ||
| private _isVisibleColumnsValid(visibleColumns: Column[]): boolean { | ||
| const getCommandColumnsCount = (): number => { |
There was a problem hiding this comment.
we can use filter and length, it would be easier to read
also looks like visibleColumns do not change during function call, so we can just calculate value, not defined function and call it later
| } | ||
|
|
||
| private _isPercentWidth(width) { | ||
| private _isPercentWidth(width: unknown): width is `${string}%` { |
There was a problem hiding this comment.
we have similar utility in grid_view: isPercentWidth, lets extract it to views/utils and use there
| items: this._getFormItemsByHiddenColumns(this._hiddenColumns), | ||
| formID: `dx-${new Guid()}`, | ||
| }; | ||
| // @ts-expect-error |
There was a problem hiding this comment.
add comment to directive, please
| options: AdaptiveDetailCellOptions, | ||
| ): void { | ||
| // eslint-disable-next-line @typescript-eslint/no-this-alias -- Form callbacks use their this | ||
| const that = this; |
There was a problem hiding this comment.
lets move alias before line 829
another option - extract bounded callbacks as variables and use them in callback
| const $dataGridElement = this.component && this.component.$element(); | ||
| private _updateFocusCore(): void { | ||
| const $dataGridElement = this.component?.$element(); | ||
| // this selector is specific to IE |
There was a problem hiding this comment.
we do not support IE, please, check, whether it is still actual
There was a problem hiding this comment.
Comment is not actual, but the code was never IE-only, so we can remove the comment, but keep the comment
| this._$focusOverlay.addClass(DX_HIDDEN); | ||
| $focusOverlay.addClass(DX_HIDDEN); | ||
| } else if ($element.length) { | ||
| // align "right bottom" for Mozilla |
There was a problem hiding this comment.
excessive comment, lets remove it
| // eslint-disable-next-line no-undef | ||
| // NOTE: this condition is for the 'Row - Redundant validation messages should not be rendered | ||
| // in a detail grid when focused row is enabled (T950174)' testcafe test. | ||
| // The detail grid is created inside document_fragment_node but it is not shadow dom |
There was a problem hiding this comment.
can we move this code to test mocks?
There was a problem hiding this comment.
I think it's better to keep it here: it's not test-only, a grid created from an HTML string with attributes ($('<div class="…">')) starts inside a document fragment, and without this branch it would listen on the fragment instead of document
No description provided.