Skip to content
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

Update types in DataView #2471

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jordankoschei-okta
Copy link
Contributor

We had some types that were incorrect in DataView; this PR fixes them.

@jordankoschei-okta jordankoschei-okta requested a review from a team as a code owner January 17, 2025 21:30
cardLayoutOptions.rowActionMenuItems as unknown as (
row: MRT_Row<MRT_RowData>,
) => any // eslint-disable-line @typescript-eslint/no-explicit-any
cardLayoutOptions.rowActionMenuItems as CardLayoutProps<MRT_RowData>["rowActionMenuItems"]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's CardLayoutProps that we need an as here? Shouldn't this be TData and not MRT_RowData?

Or even MRT_RowData<TData>?

Comment on lines +215 to +225
(row: TData, index: number, parentRow: MRT_Row<TData>) => {
if (getRowIdProp) {
return getRowIdProp(row, index, parentRow);
}
if (row.id) {
return row.id as string;
}
return createUniqueId();
},
[getRowIdProp],
) as (row: TData, index: number, parentRow?: MRT_Row<TData>) => string;
Copy link
Contributor

Choose a reason for hiding this comment

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

The as and the function look the same. We shouldn't need those here right? We should know the id is a string at this point I'd guess.

Comment on lines +28 to +29
// type RowOrTableRow<TData extends MRT_RowData> = MRT_Row<TData> | TData | MRT_Row<MRT_RowData>;
type RowOrTableRow<TData extends MRT_RowData> = MRT_Row<TData> | TData;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to keep in this commented out line?

Comment on lines +59 to +61
const row = (
incomingRow.original ? incomingRow.original : incomingRow
) as TData & { id: string };
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Is this how we're converting row to TData?

@@ -321,7 +316,7 @@ const TableLayoutContent = <TData extends MRT_RowData>({
const hasColumnWithGrow = columns.some((column) => column.grow === true);

const dataTable = useMaterialReactTable<TData>({
data: (!isEmpty && !isNoResults ? data : []) as TData[],
data: !isEmpty && !isNoResults ? data : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Did we need to remove the as TData[]? Usually you wanna say never[] or any[] to be TData[]:

Suggested change
data: !isEmpty && !isNoResults ? data : [],
data: !isEmpty && !isNoResults ? data : ([] as TData[]),

TS can't infer the type of [] without an as unless you add at least one item to it.

I wonder if we can use satisfies instead:

Suggested change
data: !isEmpty && !isNoResults ? data : [],
data: !isEmpty && !isNoResults ? data : ([] satisfies TData[]),

Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

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

Thank you so much for making these changes! You've made great strides to improving our types and removing all the TODOs I put in the last merge :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants