-
Notifications
You must be signed in to change notification settings - Fork 65
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
LG-3313: v10 Row props #1916
LG-3313: v10 Row props #1916
Conversation
…afygreen-ui into LG-3313-v10-row-props
🦋 Changeset detectedLatest commit: 727ca89 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Change: +494 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
@@ -127,6 +152,8 @@ const V11Adapter = <T extends LGRowData>({ | |||
virtualRow={ | |||
useVirtualScrolling ? (iterable as VirtualItem) : undefined | |||
} | |||
// @ts-expect-error rowProps is an additional prop passed by the `processData` function |
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 do we need to ts-expect-error here?
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.
We should fix the types in the processData
function rather than add expect error
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.
agreeed
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.
Functionally LGTM. TS types should be updated though
@@ -127,6 +152,8 @@ const V11Adapter = <T extends LGRowData>({ | |||
virtualRow={ | |||
useVirtualScrolling ? (iterable as VirtualItem) : undefined | |||
} | |||
// @ts-expect-error rowProps is an additional prop passed by the `processData` function |
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.
We should fix the types in the processData
function rather than add expect error
// @ts-expect-error | ||
<HeaderCell | ||
key={header.id} | ||
header={header} | ||
{...oldHeaderCellProps[i]} |
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.
We should update/cast the type of oldHeaderCellProps
rather than expect error
</Row> | ||
))} | ||
row.subRows.map(subRow => { | ||
// @ts-expect-error rowProps is an additional prop passed by the `processData` function |
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.
Same here
<Row key={subRow.id} row={subRow} {...subRowProps}> | ||
{subRow.getVisibleCells().map(srCell => { | ||
/* @ts-expect-error subRow.original returns the object in the user's defined shape, and should be string indexable */ | ||
const subRowCell = subRow.original[srCell.column.id](); |
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.
Seems there are a lot of issues with the TS type of row.original
vs how it's used. We should fix those
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.
agree that we should fix htis.
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changesFor new components
🧪 How to test changes