-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[core] Refactor virtualizer API #18995
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
Conversation
Deploy preview: https://deploy-preview-18995--material-ui-x.netlify.app/ Bundle size report
|
@@ -26,11 +26,17 @@ export type PinnedRows = { | |||
top: RowEntry[]; | |||
bottom: RowEntry[]; | |||
}; | |||
export namespace PinnedRows { |
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.
Codesandbox bundling seems to just drop the initializer altogether. Prior history of causing issues. Generally declared as a feature to avoid because it confuses tooling all around. While I understand the appeal of having a name refer to both a type and a value at the same time, because of aforementioned reasons I would like to ban all usage of the namespace
feature.
How do you feel about any of the following patterns:
export type PinnedRows = {
top: RowEntry[];
bottom: RowEntry[];
};
export const PINNED_ROWS = {
empty: { top: [], bottom: [] } as PinnedRows
}
// or
export const PINNED_ROWS = {
EMPTY: { top: [], bottom: [] } as PinnedRows
}
// or
export const PinnedRowsValues = {
EMPTY: { top: [], bottom: [] } as PinnedRows
}
// or
export const PINNED_ROWS_EMPTY: PinnedRows = { top: [], bottom: [] };
export type PinnedRowsType = {
top: RowEntry[];
bottom: RowEntry[];
};
export const PinnedRows = {
EMPTY: { top: [], bottom: [] } as PinnedRowsType
}
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.
So there is no way to export values and types on the same identifier without modules?
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.
not that I'm aware of.
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.
Is it just the namespace
keyword that's buggy, or using the same identifier? This works for typescript, because types and values live in separate namespaces:
export type PinnedRows = {
top: RowEntry[];
bottom: RowEntry[];
};
export const PinnedRows = {
EMPTY: { top: [], bottom: [] } as PinnedRows,
};
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.
LGTM!
packages/x-data-grid/src/hooks/features/dimensions/useGridDimensions.ts
Outdated
Show resolved
Hide resolved
Every grid demo (also on
Something to be fixed as a part of this PR? |
I prefer not to mix issues, this is just for the refactor, I'm fixing the new bugs as separate PRs. It makes it easier to review things. |
…efactor-virtualizer
Remove grid-specific references from the virtualizer API.
Changes
topPinnedHeight
andbottomPinnedHeight
to the virtualizer (by analogy toleftPinnedWidth
andrightPinnedWidth
), which the grid fills with the height of the various headers.