-
-
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
[DataGridPro] Implement header filter height #12666
Conversation
Deploy preview: https://deploy-preview-12666--material-ui-x.netlify.app/ Updated pages: |
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.
Thank you for the quick response 🚀
/** | ||
* Override the height/width of the header filters. | ||
*/ | ||
filterHeaderHeight?: number; |
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.
Also, shouldn't this prop belong to dataGridProProps.ts
like headerFilters
boolean flag?
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 need it in community for the dimensions logic.
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.
Alright, bdw, I just merged #12365
Maybe headerFilterHeight
could be placed in DataGridProcessedPropsWithShared
just like the headerFilters
boolean.
That way we won't be exposing it to MIT users yet make it accessible in the @mui/x-data-grid
scope.
Wdyt?
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.
What was the reason for splitting DataGridProcessedProps
and DataGridProcessedPropsWithShared
? Having some trouble fixing the types because DataGridProcessedProps
is used in many places where DataGridProcessedPropsWithShared
should be used. AFAIK, DataGridProcessedProps
is not used by external users so we should be able to place final typings there.
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.
What was the reason for splitting DataGridProcessedProps and DataGridProcessedPropsWithShared?
The main reason for keeping a line between DataGridProcessedProps
and DataGridProcessedPropsWithShared
in my mind was to reflect a better understanding of the specific scenario.
For example hook A and B.
A
uses some shared props from higher packages, it could be easily guessed by the reader of the code by the import name DataGridProcessedPropsWithShared
export const useA = (
apiRef: React.MutableRefObject<GridPrivateApiCommunity>,
props: Pick<
DataGridProcessedPropsWithShared,
| 'regularProp'
| 'sharedProp'
>,
): void => {
Wheras B
doesn't use a shared prop.
export const useB = (
apiRef: React.MutableRefObject<GridPrivateApiCommunity>,
props: Pick<DataGridProcessedProps, 'regularProp' | 'regularProp2'>,
): void => {
It's obvious just by looking at the first few lines of the file which hook uses shared props and which don't. It might be helpful on the understanding side for new maintainers.
But I agree that making them part of DataGridProcessedProps
along with wrapping them in Partial<>
may also do the job + simplify the maintaining the types. I do not have a hard preference on either of them. Feel free to go with the suggested refactoring.
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.
Did the refactor. Feels better this way because the typings reflect what's really present.
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.
Dropped a couple of minor comments, apart from that, it LGTM 👌
Co-authored-by: Bilal Shafi <[email protected]> Signed-off-by: Rom Grk <[email protected]>
Signed-off-by: Rom Grk <[email protected]> Co-authored-by: Bilal Shafi <[email protected]>
Signed-off-by: Rom Grk <[email protected]> Co-authored-by: Bilal Shafi <[email protected]>
Closes #12643
Add
headerFilterHeight
to control the header filters height.