ColorBox: typings comments refactoring#33682
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors ColorBox-related TypeScript typing workarounds by improving/adjusting renderer typings and updating internal ColorBox/ColorView implementations to remove several @ts-expect-error comments and tighten option/event typing.
Changes:
- Extended
dxElementWrappertypings (notablyappend(...)andval()), and aligned ColorBox option typings (fieldTemplatenullable). - Refactored ColorView/ColorBox internals to rely on the updated typings (removed multiple
@ts-expect-errorand added more explicit types). - Cleaned up several other UI components that depended on the previous
dxElementWrapper.val()typing gap.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/ui/color_box.d.ts | Allows fieldTemplate to be nullable to match runtime/default usage. |
| packages/devextreme/js/core/renderer.d.ts | Adds overloads for append(...) and introduces a getter overload for val(). |
| packages/devextreme/js/__internal/ui/text_box/text_editor.mask.ts | Removes casts/@ts-expect-error by relying on updated val() typing. |
| packages/devextreme/js/__internal/ui/text_box/text_editor.mask.strategy.ts | Removes @ts-expect-error related to val() usage. |
| packages/devextreme/js/__internal/ui/text_box/m_text_editor.base.ts | Removes @ts-expect-error comments by relying on updated val() typing. |
| packages/devextreme/js/__internal/ui/m_tag_box.ts | Removes @ts-expect-error by enabling .append(dxElementWrapper[]) via typings. |
| packages/devextreme/js/__internal/ui/m_select_box.ts | Removes @ts-expect-error tied to input .val() typing. |
| packages/devextreme/js/__internal/ui/file_uploader/file_uploader.ts | Removes @ts-expect-error around .val() usage for file input. |
| packages/devextreme/js/__internal/ui/color_box/m_color_view.ts | Refactors ColorView typing, null-handling, and removes several @ts-expect-error usages. |
| packages/devextreme/js/__internal/ui/color_box/m_color_box.ts | Refactors ColorBox typing and event handling, removing prior TS suppressions. |
Comments suppressed due to low confidence (1)
packages/devextreme/js/__internal/ui/color_box/m_color_view.ts:596
_renderHueScaleHandleconditionally createsthis._$hueScaleHandle, but then unconditionally callsgetHeight(this._$hueScaleHandle)and_placeHueScaleHandle(). If the wrapper/handle were ever absent, this would yieldundefinedheights and lead to NaN positioning. Since_renderHueScale()always creates_$hueScaleWrapper, it would be safer to remove the conditional and make the handle creation unconditional (or early-return before using the handle).
_renderHueScaleHandle(): void {
if (this._$hueScaleWrapper !== undefined) {
this._$hueScaleHandle = $('<div>')
.addClass(COLOR_VIEW_HUE_SCALE_HANDLE_CLASS)
.appendTo(this._$hueScaleWrapper);
this._createComponent(this._$hueScaleHandle, Draggable, {
contentTemplate: null,
// @ts-expect-error need to fix type for Draggable boundary option
boundary: this._$hueScaleWrapper,
allowMoveByClick: true,
dragDirection: 'vertical',
onDragMove: ({ event }) => {
this._updateByDrag = true;
this._saveValueChangeEvent(event as unknown as ValueChangedEvent);
this._updateColorHue(locate(this._$hueScaleHandle).top + this._hueScaleHandleHeight / 2);
},
});
}
this._hueScaleHandleHeight = getHeight(this._$hueScaleHandle);
this._placeHueScaleHandle();
}
a8cef25 to
0fd7f1c
Compare
838ef0b to
7a9c232
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/devextreme/js/__internal/ui/color_box/m_color_view.ts:600
_renderHueScaleHandleconditionally creates_$hueScaleHandleonly when_$hueScaleWrapperis defined, but then unconditionally callsgetHeight(this._$hueScaleHandle)and_placeHueScaleHandle(). If this guard ever triggers, it will lead to runtime errors. Since_renderHueScale()always assigns_$hueScaleWrapperbefore calling this method, it would be safer to either (1) remove theifand make_$hueScaleWrapper/_$hueScaleHandlenon-optional, or (2)returnearly when the wrapper is missing.
_renderHueScaleHandle(): void {
if (this._$hueScaleWrapper !== undefined) {
this._$hueScaleHandle = $('<div>')
.addClass(COLOR_VIEW_HUE_SCALE_HANDLE_CLASS)
.appendTo(this._$hueScaleWrapper);
this._createComponent(this._$hueScaleHandle, Draggable, {
contentTemplate: null,
// @ts-expect-error need to fix type for Draggable boundary option
boundary: this._$hueScaleWrapper,
allowMoveByClick: true,
dragDirection: 'vertical',
onDragMove: ({ event }) => {
this._updateByDrag = true;
this._saveValueChangeEvent(event);
this._updateColorHue(locate(this._$hueScaleHandle).top + this._hueScaleHandleHeight / 2);
},
});
}
this._hueScaleHandleHeight = getHeight(this._$hueScaleHandle);
this._placeHueScaleHandle();
7a9c232 to
a3e2876
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/devextreme/js/__internal/ui/color_box/m_color_view.ts:601
_renderHueScaleHandleguards handle creation withif (this._$hueScaleWrapper !== undefined), but then unconditionally usesthis._$hueScaleHandleright after (getHeight(this._$hueScaleHandle)and_placeHueScaleHandle()). If the guard ever prevents handle creation,_hueScaleHandleHeightbecomesundefinedand subsequent calculations/moves will produce NaN/invalid positioning.
Suggested fix: either remove the guard (since _renderHueScale always initializes _ $hueScaleWrapper before calling this), or return early when _ $hueScaleWrapper is missing so you don’t proceed with uninitialized handle state.
_renderHueScaleHandle(): void {
if (this._$hueScaleWrapper !== undefined) {
this._$hueScaleHandle = $('<div>')
.addClass(COLOR_VIEW_HUE_SCALE_HANDLE_CLASS)
.appendTo(this._$hueScaleWrapper);
this._createComponent(this._$hueScaleHandle, Draggable, {
contentTemplate: null,
// @ts-expect-error need to fix type for Draggable boundary option
boundary: this._$hueScaleWrapper,
allowMoveByClick: true,
dragDirection: 'vertical',
onDragMove: ({ event }) => {
this._updateByDrag = true;
this._saveValueChangeEvent(event);
this._updateColorHue(locate(this._$hueScaleHandle).top + this._hueScaleHandleHeight / 2);
},
});
}
this._hueScaleHandleHeight = getHeight(this._$hueScaleHandle);
this._placeHueScaleHandle();
}
| _valueChangeArgs(value: unknown, previousValue: unknown): { | ||
| value: unknown; | ||
| previousValue: unknown; | ||
| event?: NativeEventInfo<Editor> & ValueChangedInfo; | ||
| } { | ||
| return { | ||
| value, | ||
| previousValue, | ||
| event: this._valueChangeEventInstance, | ||
| event: this._valueChangeEventInstance as (NativeEventInfo<Editor> | ||
| & ValueChangedInfo) | undefined, | ||
| }; |
There was a problem hiding this comment.
@r-farkhutdinov and @marker-dao
i propose a more robust dealing with events in other request due relatively big size of that request and possible difficulties that might be related with those changes. What do you think?
| applyValueMode: 'useButtons', | ||
| keyStep: 1, | ||
| // @ts-expect-error ts-error | ||
| // @ts-expect-error fieldTemplate is deprecated --- IGNORE --- |
There was a problem hiding this comment.
Minor:
| // @ts-expect-error fieldTemplate is deprecated --- IGNORE --- | |
| // @ts-expect-error fieldTemplate is deprecated and the type can't be updated |
No description provided.