-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
[DataGrid] Add column filter support #380
Conversation
enabled: true, | ||
boundariesElement: 'scrollParent', | ||
}, | ||
// arrow: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't get this to work properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why display an arrow? They are never present in Material Design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have linked #201 in the issue description, added two new benchmark links, and added a screenshot for all the links showcasing the approach used for filtering.
It seems that we could consider the following:
- A filtering API, on the data only (headless)
- A quick column filter (quick filter but for each column)
- A menu column menu filter with or/and aggregation and operators (>, <, ==, etc.), Excel-like
- A chip to display the active filters, Google Ads like
@@ -11,4 +11,9 @@ export const ArrowDownwardIcon = createSvgIcon( | |||
'ArrowDownward', | |||
); | |||
|
|||
export const FilterIcon = createSvgIcon( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should retain the original name. At the minimum to remove any discussion around best naming, and at best, to keep the reference to the original icon, in case somebody looking in the source want to find it from @material-ui/icons or if the material design icon updates in the future.
export const FilterIcon = createSvgIcon( | |
export const FilterListIcon = createSvgIcon( |
enabled: true, | ||
boundariesElement: 'scrollParent', | ||
}, | ||
// arrow: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why display an arrow? They are never present in Material Design.
disablePortal={false} | ||
modifiers={{ | ||
flip: { | ||
enabled: true, | ||
}, | ||
preventOverflow: { | ||
enabled: true, | ||
boundariesElement: 'scrollParent', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried with the default options?
disablePortal={false} | |
modifiers={{ | |
flip: { | |
enabled: true, | |
}, | |
preventOverflow: { | |
enabled: true, | |
boundariesElement: 'scrollParent', | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum not sure, I forgot we are using Material-UI v4 and not v5!
[colDef], | ||
); | ||
|
||
const onFilterChange = React.useCallback((event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TextField doesn't use memo. I believe that the best way to handle performance is to only consider it once an issue Is measured. Why bother using useCallback
here?
const onFilterChange = React.useCallback((event) => { | |
const onFilterChange = (event) => { |
const [isOpen, setIsOpen] = React.useState(false); | ||
const [filterValue, setFilterValue] = React.useState(''); | ||
const [target, setTarget] = React.useState<HTMLElement | null>(null); | ||
const [colDef, setColumn] = React.useState<ColDef | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency:
const [colDef, setColumn] = React.useState<ColDef | null>(null); | |
const [colDef, setColDef] = React.useState<ColDef | null>(null); |
or
const [colDef, setColumn] = React.useState<ColDef | null>(null); | |
const [column, setColumn] = React.useState<ColDef | null>(null); |
|
||
return ( | ||
<div className={'MuiDataGrid-iconFilter'}> | ||
<IconButton aria-label="Sort" size="small" onClick={filterClick}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<IconButton aria-label="Sort" size="small" onClick={filterClick}> | |
<IconButton aria-label="filter" size="small" onClick={filterClick}> |
id={`simple-tabpanel-${index}`} | ||
aria-labelledby={`simple-tab-${index}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hard coded ids
<IconButton color="primary" aria-label="Filter" component="span"> | ||
<Search /> | ||
</IconButton> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using a regular button instead? I was confused about the icon, I thought it was a presentational one.
)} | ||
</div> | ||
{!isColumnNumeric && ( | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<> | |
<React.Fragment> |
hide={column.hideSortIcons} | ||
/> | ||
{isColumnNumeric && <ColumnHeaderFilterIcon column={column} />} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to #201