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

feat: table virtualization #2489

Open
wants to merge 78 commits into
base: master
Choose a base branch
from
Open

feat: table virtualization #2489

wants to merge 78 commits into from

Conversation

tewarig
Copy link
Contributor

@tewarig tewarig commented Jan 22, 2025

Description

Changes

Additional Information

dev checklist -
[ x ] vtable height be changed
[ x ] check why check box look weird ?
[ x ] need to make row similar to normal table
[ x ] should check the space of table.
[ x ] vtable should be aligned as normal table.
[ x ] hover actions
[ x ] sticky column
[ x ] table header
[ x ] fix ts error
[ x ] unified styles for virtualized table
[ x ] add tests

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

Comment on lines 691 to 712
* tableData={tableData}
* rowHeight={(item, index) => {
* // header height and row height
* return index === 0 ? 50 : 57.5;
* }}
* header={() => (
* <TableHeader>
* <TableHeaderRow>
* <TableHeaderCell>ID</TableHeaderCell>
* <TableHeaderCell>Amount</TableHeaderCell>
* <TableHeaderCell>Account</TableHeaderCell>
* <TableHeaderCell>Date</TableHeaderCell>
* <TableHeaderCell>Method</TableHeaderCell>
* <TableHeaderCell>Status</TableHeaderCell>
* </TableHeaderRow>
* </TableHeader>
* )}
* body={(tableItem, index) => (
* <TableRow key={index} item={tableItem}>
* <TableCell>
* <Code size="medium">{tableItem.paymentId}</Code>
Copy link
Member

Choose a reason for hiding this comment

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

This is what I was thinking-

So structurally it stays similar to our current API with few changes here and there

<Table
  tableData={tableData}
>
{(tableData) => (
  <TableVirtualizedContainer tableData={tableData}>
    <TableHeader>
      <TableHeaderRow>
      </TableHeaderRow>
    </TableHeader>
  
    <TableBody>
      {(tableItem, index) => <TableRow key={index} item={tableItem} />}
    </TableBody>
  </TableVirtualizedContainer>
)}
</Table>

OR

if we can skip the Table component and do this -

<TableVirtualized
  tableData={tableData}
>
{(tableData) => (
  <TableHeader>
    <TableHeaderRow>
    </TableHeaderRow>
  </TableHeader>

  <TableBody>
    {(tableItem, index) => <TableRow key={index} item={tableItem} />}
  </TableBody>
)}
</TableVirtualized>

Comment on lines 92 to 95
rowHeight={(item, index) => {
// header height and row height
return index === 0 ? 50 : 57.5;
}}
Copy link
Member

Choose a reason for hiding this comment

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

Should we have headerHeight internally defined? a bit confusing that index 0 here is header but index 0 in other places would be row item.

Since we know headerHeight, we can define it internally and leave rowHeight for consumers to define.

packages/blade/src/components/Table/types.ts Outdated Show resolved Hide resolved
packages/blade/src/components/Table/utils.ts Outdated Show resolved Hide resolved
'&&&': {
...styledPropsCSSObject,
},
...($styledProps?.isVirtualized && {
Copy link
Member

Choose a reason for hiding this comment

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

is this working? wouldn't it lead to ...false in some scenarios and try to destructure a boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is working... isVirtualized will be false in case of normal table

...styledPropsCSSObject,
},
...($styledProps?.isVirtualized && {
...getTableBodyStyles({
Copy link
Member

Choose a reason for hiding this comment

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

You don't have to destructure here right? I'm guessing it can just be ....(isVirtualized ? getTableBodyStyles() : {})

Copy link
Member

Choose a reason for hiding this comment

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

So in virtualized version, we add tableBodyStyles on table component?

Is there any harm if we always use tableBodyStyles here? even in non-virtualized table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, since structure is different i don't see any harm . but still we should not add any other styles which are not being used.

packages/blade/src/components/Table/Table.web.tsx Outdated Show resolved Hide resolved
packages/blade/src/components/Table/Table.web.tsx Outdated Show resolved Hide resolved
packages/blade/src/components/Table/Table.web.tsx Outdated Show resolved Hide resolved
packages/blade/src/components/Table/TableBody.web.tsx Outdated Show resolved Hide resolved
<TableHeaderCell headerKey="STATUS">Status </TableHeaderCell>
</TableHeaderRow>
</TableHeader>
<TableBody<Item>>
Copy link
Member

Choose a reason for hiding this comment

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

lets discuss this

packages/blade/src/components/Table/types.ts Outdated Show resolved Hide resolved
packages/blade/codemods/aicodemod/knowledge/Table.md Outdated Show resolved Hide resolved
/**
* isVirtualized prop determines whether the table is virutalized or not.
*/
isVirtualized?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

we can internally handle this without taking from consumer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

isVirtualized = false,
...rest
}: TableProps<Item>,
ref: React.Ref<BladeElementRef> | undefined,
Copy link
Member

Choose a reason for hiding this comment

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

We can remove ref and use height and calculate the width height of table node itself

<Box ref={parentRef}>
  <Table parentRef={parentRef} />
</Box>
<Table height="" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍🏻

{tableItem.status}
</Badge>
</TableCell>
</TableRow>
Copy link
Member

Choose a reason for hiding this comment

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

Check with footer once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked VirtualizedTable does not support footer. we can only pass header and footer. also can't think of a case where we will be using VirtualizedTable with footer.

packages/blade/src/components/Table/types.ts Outdated Show resolved Hide resolved
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.

3 participants