[BUGFIX] Table: align filter rows with table columns - #755
Conversation
Signed-off-by: Tanvi Kuchanur <alnk8465@gmail.com>
| const syncFilterRowScroll = (): void => { | ||
| filterRowInner.style.transform = `translateX(-${scrollContainer.scrollLeft}px)`; | ||
|
|
||
| setOpenFilterColumn((current) => (current === null ? current : null)); |
There was a problem hiding this comment.
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);There was a problem hiding this comment.
You are right
Simplified to a plain
setOpenFilterColumn(null).| syncColumnWidths(); | ||
|
|
||
| // Re-sync whenever the header row's size changes | ||
| const headerRow = scrollContainer.querySelector('thead tr'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You're right, it doesn't make sense to run syncColumnWidths() without a header row. Rearranged to return early
| {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}`} |
There was a problem hiding this comment.
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)
| position: 'fixed', | ||
| top: anchorEl.getBoundingClientRect().bottom + 4, | ||
| left: anchorEl.getBoundingClientRect().left, | ||
| zIndex: 1300, |
There was a problem hiding this comment.
Where does this 1300 come from? Any logic or specific reason?
There was a problem hiding this comment.
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,
|
@Aln999
Please let me know if you need help for that |
|
@Aln999 |
|
@Aln999 
|
|
I tested the table PR and your fix amazingly have made it way better! I really appreciate for the contribution. I tried to be picky and find something to complain about but I couldn't! Thank you @Aln999 🚀 |
2 participants
Description
This PR fixes the filter row alignment issue when the table is scrolled horizontally. It keeps the filter row aligned with the table columns, ensures the filter widths stay in sync, and fixes the filter search so it matches the displayed values correctly
Screenshots
Screen.Recording.2026-07-26.230445.mp4
Checklist
[<catalog_entry>] <commit message>naming convention using one of thefollowing
catalog_entryvalues:FEATURE,ENHANCEMENT,BUGFIX,BREAKINGCHANGE,DOC,IGNORE.UI Changes
#Relates to perses/perses#3835