Skip to content

feat(ColumnManagementModal): add support for extended columns #929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raswonders wants to merge 4 commits into
base: main
Choose a base branch
from
+131 −85
Open

feat(ColumnManagementModal): add support for extended columns #929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
adc7bde
chore: format files prettier & eslint auto-fix
raswonders May 7, 2026
3137822
feat: add generic parameter for appliedColumns and applyColumns
raswonders May 7, 2026
618b60c
feat: save same column shape as it was received
raswonders May 7, 2026
c8ac82e
test: preserve extension keys on columns
raswonders May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 93 additions & 42 deletions packages/module/src/ColumnManagementModal/ColumnManagementModal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import '@testing-library/jest-dom'
import { render, screen, fireEvent } from '@testing-library/react';
import '@testing-library/jest-dom';
import { render, screen, fireEvent, within } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import ColumnManagementModal, { ColumnManagementModalColumn } from './ColumnManagementModal';

const DEFAULT_COLUMNS : ColumnManagementModalColumn[] = [
const DEFAULT_COLUMNS: ColumnManagementModalColumn[] = [
{
title: 'ID',
key: 'id',
Expand Down Expand Up @@ -31,23 +31,35 @@ const DEFAULT_COLUMNS : ColumnManagementModalColumn[] = [
}
];

interface ExtendedColumn extends ColumnManagementModalColumn {
dataKey: string;
}

const EXTENDED_COLUMNS: ExtendedColumn[] = DEFAULT_COLUMNS.map((col) => ({
...col,
dataKey: `row.${col.key}`
}));

const onClose = jest.fn();
const setColumns = jest.fn();

// Simple mock to track when DragDropSort is used
jest.mock('@patternfly/react-drag-drop', () => ({
DragDropSort: ({ children }) => <div data-testid="drag-drop-sort">{children}</div>,
Droppable: ({ wrapper }) => wrapper,
Droppable: ({ wrapper }) => wrapper
}));

const renderColumnManagementModal = (props = {}) => render(<ColumnManagementModal
appliedColumns={DEFAULT_COLUMNS}
applyColumns={newColumns => setColumns(newColumns)}
isOpen
onClose={onClose}
data-testid="column-mgmt-modal"
{...props}
/>);
const renderColumnManagementModal = (props = {}) =>
render(
<ColumnManagementModal
appliedColumns={DEFAULT_COLUMNS}
applyColumns={(newColumns) => setColumns(newColumns)}
isOpen
onClose={onClose}
data-testid="column-mgmt-modal"
{...props}
/>
);

beforeEach(() => {
jest.clearAllMocks();
Expand All @@ -56,17 +68,21 @@ beforeEach(() => {

const getCheckboxesState = () => {
// Get only the column checkboxes (exclude the BulkSelect checkbox)
const checkboxes = screen.getByTestId('column-mgmt-modal').querySelectorAll('input[type="checkbox"][data-testid^="column-check-"]');
return (Array.from(checkboxes) as HTMLInputElement[]).map(c => c.checked);
}
const checkboxes = screen
.getByTestId('column-mgmt-modal')
.querySelectorAll('input[type="checkbox"][data-testid^="column-check-"]');
return (Array.from(checkboxes) as HTMLInputElement[]).map((c) => c.checked);
};

describe('ColumnManagementModal component', () => {
it('should have disabled and checked checkboxes for columns that should always be shown', () => {
expect(getCheckboxesState()).toEqual(DEFAULT_COLUMNS.map(c => c.isShownByDefault));
expect(getCheckboxesState()).toEqual(DEFAULT_COLUMNS.map((c) => c.isShownByDefault));
});

it('should have checkbox checked if column is shown by default', () => {
const idCheckbox = screen.getByTestId('column-mgmt-modal').querySelector('input[type="checkbox"][data-testid="column-check-id"]');
const idCheckbox = screen
.getByTestId('column-mgmt-modal')
.querySelector('input[type="checkbox"][data-testid="column-check-id"]');

expect(idCheckbox).toHaveAttribute('disabled');
expect(idCheckbox).toHaveAttribute('checked');
Expand All @@ -81,18 +97,20 @@ describe('ColumnManagementModal component', () => {

fireEvent.click(screen.getByText('Reset to default'));

expect(getCheckboxesState()).toEqual(DEFAULT_COLUMNS.map(c => c.isShownByDefault));
expect(getCheckboxesState()).toEqual(DEFAULT_COLUMNS.map((c) => c.isShownByDefault));
});

it('should call onReset callback when reset to default is clicked', () => {
const onResetMock = jest.fn();
render(<ColumnManagementModal
appliedColumns={DEFAULT_COLUMNS}
applyColumns={setColumns}
isOpen
onClose={onClose}
onReset={onResetMock}
/>);
render(
<ColumnManagementModal
appliedColumns={DEFAULT_COLUMNS}
applyColumns={setColumns}
isOpen
onClose={onClose}
onReset={onResetMock}
/>
);

const resetButtons = screen.getAllByText('Reset to default');
// Click the last one rendered (the new modal)
Expand All @@ -103,13 +121,15 @@ describe('ColumnManagementModal component', () => {

it('should display custom reset button label', () => {
const customLabel = 'Restaurer par défaut';
render(<ColumnManagementModal
appliedColumns={DEFAULT_COLUMNS}
applyColumns={setColumns}
isOpen
onClose={onClose}
resetToDefaultLabel={customLabel}
/>);
render(
<ColumnManagementModal
appliedColumns={DEFAULT_COLUMNS}
applyColumns={setColumns}
isOpen
onClose={onClose}
resetToDefaultLabel={customLabel}
/>
);

expect(screen.getByText(customLabel)).toBeInTheDocument();
});
Expand All @@ -124,15 +144,15 @@ describe('ColumnManagementModal component', () => {
const selectAllButton = screen.getByText('Select all (4)');
await userEvent.click(selectAllButton);

expect(getCheckboxesState()).toEqual(DEFAULT_COLUMNS.map(_ => true));
expect(getCheckboxesState()).toEqual(DEFAULT_COLUMNS.map((_) => true));
});

it('should change columns on save', () => {
fireEvent.click(screen.getByText('Impact'));
fireEvent.click(screen.getByText('Save'));

const expectedColumns = DEFAULT_COLUMNS;
const impactColumn = expectedColumns.find(c => c.title === 'Impact');
const impactColumn = expectedColumns.find((c) => c.title === 'Impact');

if (impactColumn === undefined) {
throw new Error('Expected to find "Impact" column in "DEFAULT_COLUMNS"');
Expand All @@ -153,6 +173,35 @@ describe('ColumnManagementModal component', () => {
expect(setColumns).not.toHaveBeenCalled();
});

it('should preserve extended column fields when saving', () => {
const applyColumnsMock = jest.fn();
render(
<ColumnManagementModal<ExtendedColumn>
appliedColumns={EXTENDED_COLUMNS}
applyColumns={applyColumnsMock}
isOpen
onClose={onClose}
data-testid="extended-column-modal"
/>
);

const modal = screen.getByTestId('extended-column-modal');
fireEvent.click(within(modal).getByTestId('column-check-impact'));
fireEvent.click(within(modal).getByRole('button', { name: 'Save' }));

expect(applyColumnsMock).toHaveBeenCalledTimes(1);
const saved = applyColumnsMock.mock.calls[0][0] as ExtendedColumn[];

// Test that we have same number of columns
expect(saved).toHaveLength(EXTENDED_COLUMNS.length);

// Test that extension keys were preserved
saved.forEach((col) => {
const source = EXTENDED_COLUMNS.find((aec) => aec.key === col.key);
expect(source?.dataKey).toBe(col.dataKey);
});
Copy link
Copy Markdown
Collaborator

@karelhala karelhala May 12, 2026

Choose a reason for hiding this comment

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

Suggestion: Consider replacing the forEach + individual expect with a single toEqual assertion. This is more robust — it would also catch unexpected property mutations or additions on the base fields, not just verify that dataKey survived:

expect(saved).toEqual(
  EXTENDED_COLUMNS.map((col) => ({
    ...col,
    isShown: col.key === 'impact' ? !col.isShownByDefault : col.isShownByDefault
  }))
);

This covers the full shape of every column in one shot.

Copy link
Copy Markdown
Contributor Author

@raswonders raswonders May 13, 2026

Choose a reason for hiding this comment

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

I like this suggestion, Thanks! I just used isShown: col.key === 'impact' ? !col.isShown : col.isShown, as col.isShown is the value we depend on primarily. This doesn't matter in current EXTENDED_COLUMNS setup but in some it could.

});

Copy link
Copy Markdown
Collaborator

@karelhala karelhala May 12, 2026

Choose a reason for hiding this comment

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

Suggestion: Would be good to also add a test for enableDragDrop={true} combined with extended columns — verifying that reordering + save still preserves the extra fields. The existing drag-drop tests only use DEFAULT_COLUMNS (base type), so the combination is currently untested.

Something like:

it('should preserve extended column fields after drag-drop reorder and save', () => {
  const applyColumnsMock = jest.fn();
  const reorderedExtended: ExtendedColumn[] = [
    EXTENDED_COLUMNS[3],
    EXTENDED_COLUMNS[0],
    EXTENDED_COLUMNS[2],
    EXTENDED_COLUMNS[1]
  ];
  render(
    <ColumnManagementModal<ExtendedColumn>
      appliedColumns={reorderedExtended}
      applyColumns={applyColumnsMock}
      enableDragDrop
      isOpen
      onClose={onClose}
    />
  );
  // reset + save triggers applyColumns
  const resetButtons = screen.getAllByText('Reset to default');
  fireEvent.click(resetButtons[resetButtons.length - 1]);
  fireEvent.click(screen.getAllByText('Save').pop()!);

  const saved = applyColumnsMock.mock.calls[0][0] as ExtendedColumn[];
  saved.forEach((col) => {
    expect(col.dataKey).toBeDefined();
  });
});

Copy link
Copy Markdown
Contributor Author

@raswonders raswonders May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This test and some others plus some comments are assuming resetToDefault reorders columns to default order but this is not the case. If I'm not mistaken this functionality is missing. Hence I'd skip this coverage until we have resetToDefault order ready. Or should I add it preemptively?

describe('enableDragDrop prop', () => {
it('should default enableDragDrop to false', () => {
// Default behavior should not enable drag and drop
Expand All @@ -175,16 +224,18 @@ describe('ColumnManagementModal component', () => {
DEFAULT_COLUMNS[3], // Score (last -> first)
DEFAULT_COLUMNS[0], // ID
DEFAULT_COLUMNS[2], // Impact
DEFAULT_COLUMNS[1], // Publish date
DEFAULT_COLUMNS[1] // Publish date
];

const applyColumnsMock = jest.fn();
render(<ColumnManagementModal
appliedColumns={reorderedColumns}
applyColumns={applyColumnsMock}
isOpen
onClose={onClose}
/>);
render(
<ColumnManagementModal
appliedColumns={reorderedColumns}
applyColumns={applyColumnsMock}
isOpen
onClose={onClose}
/>
);

// Click reset to default - get all buttons and click the last one
const resetButtons = screen.getAllByText('Reset to default');
Expand All @@ -197,7 +248,7 @@ describe('ColumnManagementModal component', () => {
// Verify that the saved columns match the original reordered columns order
// (after reset, it should restore the order from appliedColumns which is reorderedColumns)
expect(applyColumnsMock).toHaveBeenCalledWith(
reorderedColumns.map(col => ({
reorderedColumns.map((col) => ({
...col,
isShown: col.isShownByDefault
}))
Expand Down
81 changes: 38 additions & 43 deletions packages/module/src/ColumnManagementModal/ColumnManagementModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
import type { FunctionComponent } from 'react';
import { useState, useEffect } from 'react';
import {
Button,
Content,
ContentVariants,
ButtonVariant,
} from '@patternfly/react-core';
import { Button, Content, ContentVariants, ButtonVariant } from '@patternfly/react-core';
import { ModalProps, Modal, ModalVariant } from '@patternfly/react-core/deprecated';
import ListManager, { ListManagerItem } from '../ListManager/ListManager';

Expand All @@ -23,15 +17,16 @@ export interface ColumnManagementModalColumn {
}

/** extends ModalProps */
export interface ColumnManagementModalProps extends Omit<ModalProps, 'ref' | 'children'> {
export interface ColumnManagementModalProps<T extends ColumnManagementModalColumn = ColumnManagementModalColumn>
extends Omit<ModalProps, 'ref' | 'children'> {
/** Flag to show the modal */
isOpen?: boolean;
/** Invoked when modal visibility is changed */
onClose?: (event: KeyboardEvent | React.MouseEvent) => void;
/** Current column state */
appliedColumns: ColumnManagementModalColumn[];
appliedColumns: T[];
/** Invoked with new column state after save button is clicked */
applyColumns: (newColumns: ColumnManagementModalColumn[]) => void;
applyColumns: (newColumns: T[]) => void;
/* Modal description text */
description?: string;
/* Modal title text */
Expand All @@ -46,30 +41,32 @@ export interface ColumnManagementModalProps extends Omit<ModalProps, 'ref' | 'ch
resetToDefaultLabel?: string;
}

const ColumnManagementModal: FunctionComponent<ColumnManagementModalProps> = (
{ title = 'Manage columns',
description = 'Selected categories will be displayed in the table.',
isOpen = false,
onClose = () => undefined,
appliedColumns,
applyColumns,
ouiaId = 'ColumnManagementModal',
enableDragDrop = false,
onReset,
resetToDefaultLabel = 'Reset to default',
...props }: ColumnManagementModalProps) => {

const [ currentColumns, setCurrentColumns ] = useState(() =>
appliedColumns.map(column => ({ ...column, isShown: column.isShown ?? column.isShownByDefault }))
const ColumnManagementModal = <T extends ColumnManagementModalColumn = ColumnManagementModalColumn>({
title = 'Manage columns',
description = 'Selected categories will be displayed in the table.',
isOpen = false,
onClose = () => undefined,
appliedColumns,
applyColumns,
ouiaId = 'ColumnManagementModal',
enableDragDrop = false,
onReset,
resetToDefaultLabel = 'Reset to default',
...props
}: ColumnManagementModalProps<T>) => {
const [ currentColumns, setCurrentColumns ] = useState<T[]>(() =>
appliedColumns.map((column) => ({ ...column, isShown: column.isShown ?? column.isShownByDefault }))
);

// Sync with appliedColumns when they change
useEffect(() => {
setCurrentColumns(appliedColumns.map(column => ({ ...column, isShown: column.isShown ?? column.isShownByDefault })));
setCurrentColumns(
appliedColumns.map((column) => ({ ...column, isShown: column.isShown ?? column.isShownByDefault }))
);
}, [ appliedColumns ]);

// Convert ColumnManagementModalColumn to ListManagerItem
const listManagerItems: ListManagerItem[] = currentColumns.map(column => ({
const listManagerItems: ListManagerItem[] = currentColumns.map((column) => ({
key: column.key,
title: column.title,
isSelected: column.isShown,
Expand All @@ -79,16 +76,14 @@ const ColumnManagementModal: FunctionComponent<ColumnManagementModalProps> = (

const resetToDefault = () => {
// Reset both visibility and order to match the original appliedColumns
setCurrentColumns(appliedColumns.map(column => ({ ...column, isShown: column.isShownByDefault ?? false })));
setCurrentColumns(appliedColumns.map((column) => ({ ...column, isShown: column.isShownByDefault ?? false })));
onReset?.();
};

const updateColumns = (items: ListManagerItem[]) => {
const newColumns = currentColumns.map(column => {
const matchingItem = items.find(item => item.key === column.key);
return matchingItem
? { ...column, isShown: matchingItem.isSelected ?? column.isShownByDefault }
: column;
const newColumns = currentColumns.map((column) => {
const matchingItem = items.find((item) => item.key === column.key);
return matchingItem ? { ...column, isShown: matchingItem.isSelected ?? column.isShownByDefault } : column;
});
setCurrentColumns(newColumns);
};
Expand All @@ -103,8 +98,8 @@ const ColumnManagementModal: FunctionComponent<ColumnManagementModalProps> = (

const handleOrderChange = (items: ListManagerItem[]) => {
// Update the order of currentColumns based on the new order from ListManager
const newColumns = items.map(item => {
const originalColumn = currentColumns.find(col => col.key === item.key);
const newColumns = items.map((item) => {
const originalColumn = currentColumns.find((col) => col.key === item.key);
if (!originalColumn) {
throw new Error(`Column with key ${item.key} not found`);
}
Expand All @@ -114,13 +109,13 @@ const ColumnManagementModal: FunctionComponent<ColumnManagementModalProps> = (
};

const handleSave = (items: ListManagerItem[]) => {
const updatedColumns = items.map(item => ({
key: item.key,
title: item.title,
isShown: item.isSelected,
isShownByDefault: item.isShownByDefault,
isUntoggleable: item.isUntoggleable
}));
const updatedColumns = items.map((item) => {
const originalColumn = currentColumns.find((col) => col.key === item.key);
if (!originalColumn) {
throw new Error(`Column with key ${item.key} not found`);
}
return { ...originalColumn, isShown: item.isSelected };
});
applyColumns(updatedColumns);
onClose({} as KeyboardEvent);
};
Expand Down Expand Up @@ -158,6 +153,6 @@ const ColumnManagementModal: FunctionComponent<ColumnManagementModalProps> = (
/>
</Modal>
);
}
};

export default ColumnManagementModal;
Loading