Skip to content

[BUGFIX] Table: align filter rows with table columns #755

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
Aln999 wants to merge 1 commit into
base: main
Choose a base branch
from
+174 −78
Open

[BUGFIX] Table: align filter rows with table columns #755

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
252 changes: 174 additions & 78 deletions table/src/components/TablePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import { CellSettings, ColumnSettings, evaluateConditionalFormatting, TableOptions } from '../models';
import { buildRawTableData, getTablePanelQueryMode } from '../table-data-utils';
import { EmbeddedPanel } from './EmbeddedPanel';
import { createPortal } from 'react-dom';

Check failure on line 40 in table/src/components/TablePanel.tsx

View workflow job for this annotation

GitHub Actions / lint-npm 

`react-dom` import should occur before import of `../models`

type FilterValuesType<T> = Array<{ original: T; formatted: T }>;

Expand Down Expand Up @@ -401,6 +402,10 @@
});

const filteredDataRef = useRef<Array<Record<string, unknown>>>([]);
// Refs used to keep the filter row in sync with the table's horizontal
const panelContainerRef = useRef<HTMLDivElement>(null);
const filterRowInnerRef = useRef<HTMLDivElement>(null);
const filterCellRefs = useRef<Array<HTMLDivElement | null>>([]);

// Convert selectionMap to TanStack's RowSelectionState format
const rowSelection = useMemo((): RowSelectionState => {
Expand Down Expand Up @@ -728,6 +733,77 @@
}
}, [spec.pagination, pagination]);

// Sync the filter row's horizontal position with the table scroll.
useEffect(() => {
if (!spec.enableFiltering) {
return;
}

const scrollContainer = panelContainerRef.current?.querySelector<HTMLElement>('.MuiTableContainer-root');
const filterRowInner = filterRowInnerRef.current;

if (!scrollContainer || !filterRowInner) {
return;
}

const syncFilterRowScroll = (): void => {
filterRowInner.style.transform = `translateX(-${scrollContainer.scrollLeft}px)`;

setOpenFilterColumn((current) => (current === null ? current : null));

@shahrokni shahrokni Jul 27, 2026 •
edited
Loading

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.

It seems the set value is null anyway.
Your logic => if current is null set it to current, otherwise set it to null.
So, couldn't you simply do something like this? Am I missing something?

setOpenFilterColumn(null);

@Aln999 Aln999 Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are right
Simplified to a plain

setOpenFilterColumn(null).

setFilterAnchorEl({});
};

syncFilterRowScroll();

scrollContainer.addEventListener('scroll', syncFilterRowScroll, { passive: true });
return (): void => {
scrollContainer.removeEventListener('scroll', syncFilterRowScroll);
};
}, [spec.enableFiltering, columns, contentDimensions]);

// Sync filter cell widths with the actual rendered table column widths to keep them aligned.

Check failure on line 764 in table/src/components/TablePanel.tsx

View workflow job for this annotation

GitHub Actions / lint-npm 

Insert `··`
useEffect(() => {
if (!spec.enableFiltering) {
return;
}

const scrollContainer = panelContainerRef.current?.querySelector<HTMLElement>('.MuiTableContainer-root');
if (!scrollContainer) {
return;
}

const syncColumnWidths = (): void => {
const headerCells = scrollContainer.querySelectorAll<HTMLElement>('thead tr th');

columns.forEach((_, idx) => {
const headerCell = headerCells[idx];
const filterCell = filterCellRefs.current[idx];
if (!headerCell || !filterCell) {
return;
}

const width = `${headerCell.getBoundingClientRect().width}px`;
filterCell.style.width = width;
filterCell.style.minWidth = width;
filterCell.style.maxWidth = width;
});
};

syncColumnWidths();

// Re-sync whenever the header row's size changes

Check failure on line 794 in table/src/components/TablePanel.tsx

View workflow job for this annotation

GitHub Actions / lint-npm 

Delete `·`
const headerRow = scrollContainer.querySelector('thead tr');

@shahrokni shahrokni Jul 27, 2026 •
edited
Loading

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.

You could've lifted this line up next to the

const scrollContainer = panelContainerRef.current?.querySelector<HTMLElement>('.MuiTableContainer-root');
const headerRow = scrollContainer?.querySelector('thead tr');

if(!headerRow){
   return;
}

With an early return you could've avoided the execution of some lines and also unnecessary heap memory consumption. WDYT? But before taking any action, does it make sense to initially call syncColumnWidths(); even if headerRow is undefined? Because, if this is the case, my suggestion does not make any sense.

@Aln999 Aln999 Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You're right, it doesn't make sense to run syncColumnWidths() without a header row. Rearranged to return early

const resizeObserver = new ResizeObserver(syncColumnWidths);
if (headerRow) {
resizeObserver.observe(headerRow);
}

return (): void => {
resizeObserver.disconnect();
};
}, [spec.enableFiltering, columns, contentDimensions, selectionEnabled, actionButtons]);


Check failure on line 806 in table/src/components/TablePanel.tsx

View workflow job for this annotation

GitHub Actions / lint-npm 

Delete `⏎`
if (contentDimensions === undefined) {
return null;
}
Expand All @@ -748,99 +824,119 @@
}

return (
<>
<div ref={panelContainerRef} style={{ display: 'contents' }}>
{confirmDialog}
{spec.enableFiltering && (
<div
style={{
display: 'flex',
overflow: 'hidden',
background: theme.palette.background.default,
borderBottom: `1px solid ${theme.palette.divider}`,
width: contentDimensions.width,
boxSizing: 'border-box',
}}
>

Check failure on line 838 in table/src/components/TablePanel.tsx

View workflow job for this annotation

GitHub Actions / lint-npm 

Delete `⏎`
{columns.map((column, idx) => {
const filters = getSelectedFilterValues(column.accessorKey as string);
const columnWidth = column.width || spec.defaultColumnWidth;
return (
<div
key={`filter-${idx}`}
style={{
padding: '8px',
borderRight: idx < columns.length - 1 ? `1px solid ${theme.palette.divider}` : 'none',
width: columnWidth,
minWidth: columnWidth,
maxWidth: columnWidth,
display: 'flex',
alignItems: 'center',
position: 'relative',
boxSizing: 'border-box',
flex: typeof columnWidth === 'number' ? 'none' : '1 1 auto',
}}
>
<span
style={{
marginRight: 8,
fontSize: '12px',
color: theme.palette.text.secondary,
flex: 1,
overflow: 'hidden',
textOverflow: 'ellipsis',
whiteSpace: 'nowrap',
}}
>
{filters.length ? `${filters.length} items` : 'All'}
</span>
<button
onClick={(e) => {
handleFilterClick(e, column.accessorKey as string);

<div
ref={filterRowInnerRef}
style={{
display: 'flex',
width: 'max-content',
willChange: 'transform',
}}
>
{columns.map((column, idx) => {
const filters = getSelectedFilterValues(column.accessorKey as string);
const columnWidth = column.width || spec.defaultColumnWidth;
const anchorEl = filterAnchorEl[column.accessorKey as string];

return (
<div
key={`filter-${idx}`}
Comment on lines +848 to +855

@shahrokni shahrokni Jul 27, 2026 •
edited
Loading

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.

I know this is the legacy part. But let's see if we can improve it.
Although the code is looping over the columns the return element key has been named filter-${idx}
Probably the key should've included column-${something}
Now, lets say my assumption is correct (we need to check that first)
Then the key is not really performant. According to official React docs, assigning an index-based key is not a good idea. So, couldn't the original developer utilize column.accessorKey, it seems it is unique. Let's take a look and see if we can improve this key carefully. (Make sure accessorKey is unique)

ref={(el) => {
filterCellRefs.current[idx] = el;
}}
style={{
border: `1px solid ${theme.palette.divider}`,
background: theme.palette.background.paper,
cursor: 'pointer',
fontSize: '12px',
color: filters.length ? theme.palette.primary.main : theme.palette.text.secondary,
padding: '4px 8px',
borderRadius: '4px',
minWidth: '20px',
height: '24px',
flexShrink: 0,
transition: 'all 0.2s ease',
}}
onMouseEnter={(e) => {
e.currentTarget.style.background = theme.palette.action.hover;
}}
onMouseLeave={(e) => {
e.currentTarget.style.background = theme.palette.background.paper;
padding: '8px',
borderRight: idx < columns.length - 1 ? `1px solid ${theme.palette.divider}` : 'none',
// These are just a starting size for the first paint —
// the width-sync effect above overwrites them with the
// table's actual rendered column widths.
width: columnWidth,
minWidth: columnWidth,
maxWidth: columnWidth,
display: 'flex',
alignItems: 'center',
position: 'relative',
boxSizing: 'border-box',
flex: 'none',
}}
type="button"
>
▼
</button>

{openFilterColumn === column.accessorKey && (
<div
<span
style={{
marginRight: 8,
fontSize: '12px',
color: theme.palette.text.secondary,
flex: 1,
overflow: 'hidden',
textOverflow: 'ellipsis',
whiteSpace: 'nowrap',
}}
>
{filters.length ? `${filters.length} items` : 'All'}
</span>
<button
onClick={(e) => {
handleFilterClick(e, column.accessorKey as string);
}}
style={{
position: 'absolute',
top: '100%',
left: 0,
zIndex: 1000,
marginTop: 4,
border: `1px solid ${theme.palette.divider}`,
background: theme.palette.background.paper,
cursor: 'pointer',
fontSize: '12px',
color: filters.length ? theme.palette.primary.main : theme.palette.text.secondary,
padding: '4px 8px',
borderRadius: '4px',
minWidth: '20px',
height: '24px',
flexShrink: 0,
transition: 'all 0.2s ease',
}}
onMouseEnter={(e) => {
e.currentTarget.style.background = theme.palette.action.hover;
}}
onMouseLeave={(e) => {
e.currentTarget.style.background = theme.palette.background.paper;
}}
type="button"
>
<ColumnFilterDropdown
allValues={columnUniqueValues[column.accessorKey as string] || []}
selectedValues={filters}
onFilterChange={(values) => updateColumnFilter(column.accessorKey as string, values)}
theme={theme}
/>
</div>
)}
</div>
);
})}
▼
</button>

{openFilterColumn === column.accessorKey &&
anchorEl &&
createPortal(
<div
style={{
position: 'fixed',
top: anchorEl.getBoundingClientRect().bottom + 4,
left: anchorEl.getBoundingClientRect().left,
zIndex: 1300,

@shahrokni shahrokni Jul 27, 2026

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.

Where does this 1300 come from? Any logic or specific reason?

@Aln999 Aln999 Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

no good reason to hard-code it. It happens to match MUI's own theme.zIndex.modal so I've referenced that directly instead of the magic number. Will update it to

zIndex: theme.zIndex.modal,

}}
>
<ColumnFilterDropdown
allValues={columnUniqueValues[column.accessorKey as string] || []}
selectedValues={filters}
onFilterChange={(values) => updateColumnFilter(column.accessorKey as string, values)}
theme={theme}
/>
</div>,
document.body
)}
</div>
);
})}
</div>
</div>
)}
<Table
Expand All @@ -862,6 +958,6 @@
getItemActions={({ id, data }) => getItemActionButtons({ id, data: data as Record<string, unknown> })}
hasItemActions={actionButtons && actionButtons.length > 0}
/>
</>
</div>
);
}
}

Check failure on line 963 in table/src/components/TablePanel.tsx

View workflow job for this annotation

GitHub Actions / lint-npm 

Insert `⏎`
Loading