Repository navigation
Conversation
b00b197 to
3800964
Compare
| each(items, function () { | ||
| if (this.selected) { | ||
| selectedItems.push(this); | ||
| each(items, (_, item) => { |
There was a problem hiding this comment.
can be replaced with items.filter
| private applyHeaderFilter(options: HeaderFilterViewOptions): void { | ||
| // @ts-expect-error the list is created here | ||
| const list: List | TreeView = this.getListComponent(); | ||
| const searchValue = (list as List).option('searchValue'); |
There was a problem hiding this comment.
can we replace as List and as TreeView below with type guard and remove directives?
| const filterValues: HeaderFilterValues = []; | ||
|
|
||
| const fillSelectedItemKeys = function (filterValues, items, isExclude) { | ||
| const fillSelectedItemKeys = ( |
There was a problem hiding this comment.
lets extract as utility
|
|
||
| const popupContainer = that.getPopupContainer(); | ||
| // @ts-expect-error the popup is created here | ||
| const popupContainer: Popup = this.getPopupContainer(); |
There was a problem hiding this comment.
if we do know popup already created here, can we replace expect-error with guard?
There was a problem hiding this comment.
I fixed your comment differently: _initializePopupContainer now returns popup, so the additional directive and check are no longer needed.
| that.getListComponent().focus(); | ||
| onShown: (): void => { | ||
| // @ts-expect-error the list is created here | ||
| this.getListComponent().focus(); |
There was a problem hiding this comment.
can we use ?. here instead of directive?
| this.getListComponent().focus(); | |
| this.getListComponent()?.focus(); |
| showSelectionControls: true, | ||
| selectionMode: needShowSelectAllCheckbox ? 'all' as const : 'multiple' as const, | ||
| onOptionChanged: onListOptionChanged, | ||
| onSelectionChanged(event) { |
There was a problem hiding this comment.
lets extrast to separate method/utility, as it is too big to define as object field
There was a problem hiding this comment.
Ideally, this should be extracted into a utility method and refactored. However, this function also uses other helper methods from the same module, so doing this would result in quite extensive changes. As far as I remember, we wanted to avoid such changes as part of the typing effort.
| // @ts-expect-error search is set below, the default options provide texts | ||
| const headerFilterOptions: HeaderFilterOptions = extend( | ||
| true, | ||
| {}, | ||
| generalHeaderFilter, | ||
| specificHeaderFilter, | ||
| ); | ||
|
|
||
| headerFilterOptions.search ??= {}; |
There was a problem hiding this comment.
| // @ts-expect-error search is set below, the default options provide texts | |
| const headerFilterOptions: HeaderFilterOptions = extend( | |
| true, | |
| {}, | |
| generalHeaderFilter, | |
| specificHeaderFilter, | |
| ); | |
| headerFilterOptions.search ??= {}; | |
| const headerFilterOptions: HeaderFilterOptions = extend( | |
| true, | |
| { | |
| search: {}, | |
| }, | |
| generalHeaderFilter, | |
| specificHeaderFilter, | |
| ); |
There was a problem hiding this comment.
This could introduce an unintended behavior change:
Currently, the ??= operator replaces null with {}. However, if the target object is { search: {} }, extend overwrites its value with null. As a result, accessing headerFilterOptions.search.enabled, for example, throws an error.
No description provided.