feat(ui): Update Table with Fixed Height#364
feat(ui): Update Table with Fixed Height#364kylengn merged 1 commit intofeat/update-modal-body-headerfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis pull request extends the ModalBodyHeader component with support for additional children content, implements dynamic table height calculations for consistent layout management, adds inline style support to the table wrapper, and adjusts test data sample size. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/react/ui/src/components/table/table.tsx (1)
6-14: Style is applied to both wrapper and table element.The
styleprop is applied to the wrapperdiv(line 8) but is also passed through to the<table>via{...props}(line 13). This meansminHeight(and any other inline styles) will be applied to both elements, which is likely unintended.Consider destructuring
styleout of props:🐛 Proposed fix
-function Table({className, ...props}: ComponentProps<'table'>) { +function Table({className, style, ...props}: ComponentProps<'table'>) { return ( <div className="relative w-full overflow-auto scrollbar rounded-x-8 rounded-b-8" - style={props.style} + style={style} > <table data-slot="table" className={cn('w-full caption-bottom text-sm', className)} {...props} /> </div> ); }libs/react/ui/src/components/table/data-table.tsx (1)
233-251: Skeleton column count mismatch when selection is enabled.The loading skeleton uses
columns.map()(line 235, 245), but whenshowSelectedCountistrue, the actual table usescolumnsWithSelectionwhich includes an additional selection column. This will cause the skeleton to render one fewer column than the loaded table.🐛 Proposed fix
<TableHeader> <TableRow className="hover:bg-transparent border-b"> - {columns.map((_, idx) => ( + {columnsWithSelection.map((_, idx) => ( <TableHead key={idx.toString()}> <Skeleton className="h-16 w-24" /> </TableHead> ))} </TableRow> </TableHeader> <TableBody> {Array.from({length: skeletonRowCount}).map((_, rowIdx) => ( <TableRow key={rowIdx.toString()} className="hover:bg-transparent"> - {columns.map((_, colIdx) => ( + {columnsWithSelection.map((_, colIdx) => ( <TableCell key={colIdx.toString()}> <Skeleton className="h-16 w-full" /> </TableCell> ))} </TableRow> ))} </TableBody>
🧹 Nitpick comments (2)
.changeset/wild-rabbits-kick.md (1)
1-5: Changeset description is incomplete.The changeset only mentions the
ModalBodyHeaderupdate, but this PR also includes significant changes to theDataTablecomponent (dynamic height calculations, pagination logic, empty row handling) and theTablecomponent (inline style support). Consider updating the description to reflect all user-facing changes.libs/react/ui/src/components/table/data-table.tsx (1)
216-224: Consider extracting height constants.The magic numbers for heights (40, 40, 48) could be extracted to named constants at the module level for better maintainability and self-documentation.
♻️ Suggested refactor
+const TABLE_HEADER_HEIGHT = 40; +const TABLE_ROW_HEIGHT = 40; +const TABLE_PAGINATION_HEIGHT = 48; + export function DataTable<TData, TValue>({ // ... }) { // ... - const headerHeight = 40; - const rowHeight = 40; - const paginationHeight = 48; + const headerHeight = TABLE_HEADER_HEIGHT; + const rowHeight = TABLE_ROW_HEIGHT; + const paginationHeight = TABLE_PAGINATION_HEIGHT;
There was a problem hiding this comment.
Pull request overview
This pull request implements a fixed-height table feature for paginated tables and adds a children prop to the ModalBodyHeader component. The main changes focus on maintaining consistent table height across pagination by adding empty placeholder rows when the current page has fewer rows than the page size.
Changes:
- Implemented fixed-height table with minimum height calculation based on header, rows, and pagination footer heights
- Added empty placeholder rows to fill remaining space when current page has fewer rows than page size
- Forwarded style prop to table wrapper div to support custom height styling
- Added
childrenprop toModalBodyHeadercomponent for additional content flexibility - Reduced test data count from 100 to 51 rows for better demonstration of pagination
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/react/ui/src/components/table/table.tsx | Added style prop forwarding to wrapper div for custom height support |
| libs/react/ui/src/components/table/data-table.tsx | Implemented fixed-height table logic with empty row padding and height calculations |
| libs/react/ui/src/components/modal/modal.tsx | Added children prop to ModalBodyHeader component |
| libs/react/ui/src/components/table/table.stories.data.ts | Reduced sample data count from 100 to 51 for pagination testing |
| .changeset/wild-rabbits-kick.md | Added changeset for version tracking |
Comments suppressed due to low confidence (1)
libs/react/ui/src/components/table/table.tsx:13
- The style prop is being applied to the wrapping div, but props (which includes style) is also spread onto the table element. This means if a style prop is passed, it will be applied to both the div wrapper and the table element itself, which may not be the intended behavior. Consider either extracting the style prop from props before spreading, or documenting that style is meant to apply to the wrapper div only.
<div
className="relative w-full overflow-auto scrollbar rounded-x-8 rounded-b-8"
style={props.style}
>
<table
data-slot="table"
className={cn('w-full caption-bottom text-sm', className)}
{...props}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| --- | ||
| "@shipfox/react-ui": minor | ||
| --- | ||
|
|
||
| Update ModalBodyHeader component |
There was a problem hiding this comment.
The changeset only mentions "Update ModalBodyHeader component" but the PR title is "Feat/update fixed height table" and includes significant changes to the table components (fixed height implementation, empty row padding, etc.). The changeset should describe all the changes in this PR, not just the modal update.
| const headerHeight = 40; | ||
| const rowHeight = 40; | ||
| const paginationHeight = 48; |
There was a problem hiding this comment.
The hardcoded height values (headerHeight = 40, rowHeight = 40, paginationHeight = 48) are magic numbers that could become out of sync with the actual CSS styling. Consider extracting these as named constants at the module level with comments explaining how they align with the CSS classes (e.g., h-40 for header, py-10 + leading-20 for rows), or consider using CSS variables or a more maintainable approach to ensure these values stay synchronized with the actual rendered heights.
| <TableCell | ||
| colSpan={table.getAllColumns().length} | ||
| className="h-240 text-center rounded-t-8" | ||
| className="h-240 text-center rounded-t-8 hover:bg-transparent" |
There was a problem hiding this comment.
The className includes both "hover:bg-transparent" in the TableRow and then again in the nested TableCell className. While this doesn't break anything, the hover:bg-transparent on the TableCell is redundant since it's already applied to the parent TableRow. Consider removing the duplicate from the TableCell className.
| className="h-240 text-center rounded-t-8 hover:bg-transparent" | |
| className="h-240 text-center rounded-t-8" |
| {table.getAllColumns().map((column) => ( | ||
| <TableCell key={column.id}> | ||
| <span className="invisible">.</span> | ||
| </TableCell> | ||
| ))} |
There was a problem hiding this comment.
The empty rows use table.getAllColumns() to generate cells, while actual data rows use row.getVisibleCells(). If column visibility is toggled, this could result in a mismatch where empty rows have more cells than data rows. Consider using table.getVisibleLeafColumns() or a similar approach to ensure empty rows match the visible column count of data rows.
kylengn
changed the title
* feat(modal): add children prop to ModalBodyHeader for flexible content rendering * feat(ui): update ModalBodyHeader component * feat(ui): remove hover state of empty table * feat(ui): enhance DataTable component with dynamic row height and pagination handling (#364) * feat(ui): update ModalBodyHeader and Table components for improved functionality * feat(ui): adjust row and pagination heights in DataTable and update TableCell styles for better layout
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.