Skip to content

Grids: type m_column_chooser - #35951

Open
Alyar666 wants to merge 9 commits into
DevExpress:mainfrom
Alyar666:grid_core_type_column_chooser_26_2
Open

Alyar666 wants to merge 9 commits into
DevExpress:mainfrom
Alyar666:grid_core_type_column_chooser_26_2

Conversation

@Alyar666

Copy link
Copy Markdown
Contributor

No description provided.

@Alyar666
Alyar666 requested a review from a team as a code owner October 26, 2026 22:45
@Alyar666 Alyar666 added the 26_2 label Oct 26, 2026
@Alyar666 Alyar666 self-assigned this Oct 26, 2026
import { defaultOptions } from './const';
import type { ColumnChooserItem, NodeInternal } from './types';

const COLUMN_CHOOSER_CLASS = 'column-chooser';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, extract all class constants to a separate file and combine them into object variable, then use this variable in all modules that uses same variables - like we made in rows_view and other views


const processItems = function (that: ColumnChooserView, chooserColumns) {
const items: any = [];
const processItems = (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convert to method, please

}

// @ts-expect-error the changed columns exist
const changedColumns: Column[] | undefined = columnIndices

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we replace this directive with guard? for example filtering result array so only defined values left?
then we can remove type definition and directive

Comment on lines +440 to 441
const needUpdate: boolean | undefined = hasItemsOptionNames
|| (changes.changeTypes.columns && optionNames.all);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const needUpdate: boolean | undefined = hasItemsOptionNames
|| (changes.changeTypes.columns && optionNames.all);
const needUpdate = hasItemsOptionNames || !!(changes.changeTypes.columns && optionNames.all);

result.push(item);
});
if (!$nodes) {
return $([]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use simpler version - it makes the same, but easier to read

Suggested change
return $([]);
return $();

Comment on lines +538 to 541
public isColumnChooserVisible(): boolean | undefined {
const popupContainer = this._popupContainer;

return popupContainer?.option('visible');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if this method returned a plain boolean? Then the !! in the ColumnChooserColumnHeadersExtender.allowDragging override can go away, and callers get a narrower type

Suggested change
public isColumnChooserVisible(): boolean | undefined {
const popupContainer = this._popupContainer;
return popupContainer?.option('visible');
public isColumnChooserVisible(): boolean {
const popupContainer = this._popupContainer;
return Boolean(popupContainer?.option('visible'));

const isSelectMode = this.isSelectMode();
const chooserColumns = this._columnsController.getChooserColumns(isSelectMode);
const $content = this._popupContainer?.$content();
const $nodes = $content?.find('.dx-treeview-node');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the class constants are being reworked anyway: TreeView already exports NODE_CLASS from tree_view.base.ts, and grid code already imports from that module in header_filter/utils.ts. Maybe use .${NODE_CLASS} here instead of the hard-coded string?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants