ColorBox: typings comments refactoring#33682
Open
vorobey wants to merge 4 commits into
Open
Conversation
Contributor
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();
}
|
|
||
| this._updateByDrag = false; | ||
| super._optionChanged({ ...args, value: this.option('value') }); | ||
| super._optionChanged({ ...args, value: this.option('value') as string }); |
Comment on lines
205
to
215
| target: this._input(), | ||
| onEnterKeyPressed({ event }) { | ||
| that._colorViewEnterKeyPressed = true; | ||
| if (that._colorView.option('value') !== that.option('value')) { | ||
| that._saveValueChangeEvent(event); | ||
| that._applyNewColor(that._colorView.option('value')); | ||
| that.close(); | ||
| onEnterKeyPressed: ({ event }: ValueChangedEvent): void => { | ||
| const { value: optionValue } = this.option(); | ||
| this._colorViewEnterKeyPressed = true; | ||
| if (this._colorView.option('value') !== optionValue) { | ||
| this._saveValueChangeEvent(event as ValueChangedEvent | undefined); | ||
| const { value: colorViewValue } = this._colorView.option(); | ||
| this._applyNewColor(colorViewValue); | ||
| this.close(); | ||
| } | ||
| }, |
Comment on lines
+269
to
275
| _applyButtonHandler(e: ValueChangedEvent): void { | ||
| this._saveValueChangeEvent(e.event as unknown as ValueChangedEvent); | ||
| const { value } = this._colorView.option(); | ||
| this._applyNewColor(value); | ||
|
|
||
| super._applyButtonHandler(); | ||
| } |
Comment on lines
+380
to
387
| _applyColorFromInput(value: string): string { | ||
| const { editAlphaChannel, value: optionValue } = this.option(); | ||
| const newColor = new Color(value); | ||
|
|
||
| if (newColor.colorIsInvalid) { | ||
| this._resetInputValue(); | ||
| return this.option('value'); | ||
| return optionValue as string; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.