feat(Table): implement table component in Design System #1438
Conversation
WalkthroughThis pull request introduces a comprehensive table component for a React application, consisting of several files that define its structure, styling, and functionality. Key components include Changes
Assessment against linked issues
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
arturfracala
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (22)
packages/react-components/src/components/Table/styles/size.scss (2)
1-10: LGTM with suggestions for improved robustnessConsider adding parameter validation and handling nested tables:
@mixin table-size($height: 48px, $padding-v: 8px, $padding-h: 16px) { // Scope to immediate children to handle nested tables > tr { height: $height; } > tr > th > span:first-child, > tr > td { padding: $padding-v $padding-h; } }
1-4: Consider using CSS custom properties for dynamic updatesReplace hardcoded values with CSS custom properties for better maintainability:
@mixin table-size($height, $padding-v, $padding-h) { tr { height: var(--table-row-height, $height); } }packages/react-components/src/components/Table/TableRow.module.scss (1)
4-11: Consider optimizing border handling.The current border implementation might create double borders between stacked rows. Consider using
border-collapse: collapsein the parent table or using a more robust border strategy..#{$base-class} { - border-bottom: 1px solid var(--border-basic-tertiary); + &:not(:last-child) { + border-bottom: 1px solid var(--border-basic-tertiary); + } background-color: var(--background-01); &:not(.#{$table-class}__header):hover { background-color: var(--surface-primary-hover); } }packages/react-components/src/components/Table/styles/pin.scss (1)
1-9: Consider using z-index variable for better maintainabilityThe implementation looks good, but consider extracting the z-index value to a variable to maintain consistency across components.
+$table-header-z-index: 5; + @mixin generatePin($type) { @if $type == 'header' { thead { position: sticky; top: 0; - z-index: 5; + z-index: $table-header-z-index; box-shadow: 0 12px 12px -7px #1313172e; } }packages/react-components/src/components/Table/types.ts (3)
1-6: Consider adding width configuration to Column typeThe Column type should include width-related properties to support column resizing functionality mentioned in ITableProps.
export type Column<T> = { key: keyof T; header: string; sortable?: boolean; sortValue?: (item: T) => string | number; + width?: number | string; + minWidth?: number; + maxWidth?: number; };
8-10: Consider expanding PinOptions for column pinningFor advanced table functionality, consider supporting left/right column pinning.
-export type PinOptions = 'header'; +export type PinOptions = 'header' | 'left' | 'right';
31-34: Add JSDoc comments for better documentationConsider adding documentation to explain the purpose of null key.
+/** + * Configuration for table sorting + * @property key - Column key to sort by, null means no sorting + * @property direction - Sort direction (ascending/descending/none) + */ export interface SortConfig<T> { key: keyof T | null; direction: SortOrder; }packages/react-components/src/components/Table/TableBody.tsx (1)
4-12: Add TypeScript documentation and improve type safety.Consider adding JSDoc comments and refining types:
+/** Props for the TableBody component */ interface TableBodyProps<T> { + /** Array of data items to display in the table */ data: T[]; + /** Column configuration array */ columns: Column<T>[]; + /** Function to get unique identifier for each row */ getRowId: (row: T) => string | number; + /** Array of column widths */ columnWidths: (number | undefined)[]; + /** Enable row selection */ selectable?: boolean; + /** Check if a row is selected */ - isSelected: (id: string | number) => boolean; + isSelected: selectable extends true ? (id: string | number) => boolean : never; + /** Toggle row selection */ - toggleRowSelection: (id: string | number) => void; + toggleRowSelection: selectable extends true ? (id: string | number) => void : never; }packages/react-components/src/components/Table/TableRow.tsx (2)
9-9: Consider using a more specific baseClass nameThe current
baseClassname 'row' is too generic and could clash with other CSS classes. Consider using 'tableRow' or 'dsTableRow' to be more specific to the Design System context.-const baseClass = 'row'; +const baseClass = 'tableRow';
11-19: Add JSDoc documentation for the interface and consider stricter typingThe interface would benefit from documentation explaining its purpose and props. Also, consider using a more specific type for rowId to ensure consistency.
+/** + * Props for the TableRow component + * @template T - The type of data represented in the row + */ interface TableRowProps<T> { + /** The data object for this row */ row: T; + /** Column definitions specifying how to render each field */ columns: Column<T>[]; + /** Width in pixels for each column */ columnWidths: (number | undefined)[]; + /** Whether the row can be selected */ selectable?: boolean; + /** Function to check if a row is selected */ isSelected: (id: string | number) => boolean; + /** Function to toggle row selection */ toggleRowSelection: (id: string | number) => void; - rowId: string | number; + /** Unique identifier for the row */ + rowId: string; }packages/react-components/src/components/Table/TableHeader.module.scss (2)
27-31: Avoid using !importantUsing
!importantbreaks CSS specificity chains. Consider increasing selector specificity instead.- background-color: var(--background-01) !important; +.#{$base-class} th#{&} { + background-color: var(--background-01); +}
11-11: Use design system tokens for font-sizeReplace hardcoded
13pxwith an appropriate design system token for consistent typography.- font-size: 13px; + font-size: var(--font-size-sm);packages/react-components/src/components/Table/Table.module.scss (4)
1-9: Add border-spacing fallback for better browser compatibility.#{$base-class} { width: 100%; + border-spacing: 0; border-collapse: collapse;
10-20: Document size mixin parametersAdd comments explaining the parameters (height, padding, font-size) for better maintainability.
&--small { + // @param1: row-height (48px) + // @param2: cell-padding (8px) + // @param3: font-size (16px) @include size.table-size(48px, 8px, 16px); }
32-49: Simplify nested selectors for better maintainability&__selected { display: flex; gap: var(--spacing-1); align-items: center; padding: var(--spacing-2); font-weight: 500; - > p { + p { margin: 0; - font-weight: 500; } - > div:first-child { + .icon-wrapper { display: flex; align-items: center; margin-right: var(--spacing-1); } }
65-73: Add vertical alignment for consistent cell contenttd, th { text-align: left; + vertical-align: middle; }packages/react-components/src/components/Table/hooks.ts (1)
41-49: Optimize performance for large datasetsConsider memoizing allRowIds to avoid recreating the Set on every toggle.
+ const allRowIds = useMemo( + () => new Set(data.map(getRowId)), + [data, getRowId] + ); const toggleSelectAll = useCallback(() => { - const allRowIds = new Set(data.map(getRowId)); const allSelected = selectedRows?.size === data.length; const updatedSelectedRows = allSelected ? new Set<string | number>() - : allRowIds; + : new Set(allRowIds); onSelectionChange?.(updatedSelectedRows); - }, [data, getRowId, selectedRows, onSelectionChange]); + }, [allRowIds, selectedRows, onSelectionChange]);packages/react-components/src/components/Table/TableHeader.tsx (2)
21-31: Add JSDoc comments to document the interface props.While the types are well-defined, adding JSDoc comments would improve maintainability and developer experience.
+/** + * Props for the TableHeader component + * @template T - The type of data being displayed in the table + */ interface TableHeaderProps<T> { + /** Array of column definitions */ columns: Column<T>[]; + /** Current sort configuration */ sortConfig: { key: keyof T | null; direction: SortOrder }; + /** Callback for handling sort changes */ handleSort: (key: keyof T) => void; resizable?: boolean; handleMouseDown: (index: number, event: React.MouseEvent) => void; columnRefs: React.RefObject<(HTMLTableCellElement | null)[]>; selectable?: boolean; hoveredColumnIndex: number | null; setHoveredColumnIndex: (index: number | null) => void; }
63-64: Memoize event handlers to prevent unnecessary re-renders.The onClick handler is recreated on every render.
+const handleColumnSort = React.useCallback( + (key: keyof T) => () => handleSort(key), + [handleSort] +); -onClick={() => handleSort(column.key)} +onClick={handleColumnSort(column.key)}packages/react-components/src/components/Table/Table.tsx (3)
65-65: Translate inline comment to English.The comment is in Polish. Please use English for all comments for consistency.
147-150: Implement indeterminate state for partial selection.When some rows are selected but not all, the select-all checkbox should display an indeterminate state to improve user experience.
39-40: Simplify object properties using shorthand syntax.You can use property shorthand for
selectedRowsandonSelectionChange.Apply this diff:
selectedRows: selectedRows, onSelectionChange: onSelectionChange, + // Simplify to: + selectedRows, + onSelectionChange,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/react-components/src/components/Table/Table.module.scss(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.module.scss(1 hunks)packages/react-components/src/components/Table/TableHeader.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.module.scss(1 hunks)packages/react-components/src/components/Table/TableRow.tsx(1 hunks)packages/react-components/src/components/Table/hooks.ts(1 hunks)packages/react-components/src/components/Table/index.ts(1 hunks)packages/react-components/src/components/Table/styles/pin.scss(1 hunks)packages/react-components/src/components/Table/styles/size.scss(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/react-components/src/components/Table/index.ts
🔇 Additional comments (13)
packages/react-components/src/components/Table/TableRow.module.scss (1)
1-2: LGTM! Clear and maintainable variable naming.
The SCSS variables follow a clear naming convention that aligns with component structure.
packages/react-components/src/components/Table/types.ts (1)
12-16: LGTM!
The SortOrder enum is well-defined with clear values.
packages/react-components/src/components/Table/TableBody.tsx (1)
1-2: LGTM! Clean and focused imports.
packages/react-components/src/components/Table/TableHeader.module.scss (2)
12-12: Verify accessibility requirements
user-select: none might impact accessibility. Ensure this aligns with accessibility requirements, especially for screen readers and keyboard navigation.
1-3: Verify usage of imported style modules
The imported pin and size modules don't appear to be used in this file.
✅ Verification successful
Imports are correctly used in the table component
The imported pin and size modules are properly utilized:
sizemodule provides table sizing mixins used in--small,--medium, and--largemodifierspinmodule provides header pinning functionality used in--pinned_headermodifier
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of pin and size styles in related files
rg -l "pin\." "packages/react-components/src/components/Table/"
rg -l "size\." "packages/react-components/src/components/Table/"
Length of output: 258
Script:
#!/bin/bash
# Check the content of Table.module.scss to see how pin and size are used
cat "packages/react-components/src/components/Table/Table.module.scss"
Length of output: 1177
Script:
#!/bin/bash
# Check if the imported style modules exist and their content
cat "packages/react-components/src/components/Table/styles/pin.scss" 2>/dev/null
cat "packages/react-components/src/components/Table/styles/size.scss" 2>/dev/null
Length of output: 566
packages/react-components/src/components/Table/Table.module.scss (3)
22-24: Verify sticky header browser support
The sticky header implementation needs verification across different browsers.
✅ Verification successful
Sticky header implementation is browser-safe
The implementation uses standard position: sticky without vendor prefixes, which is well-supported across modern browsers (IE11+). The mixin includes proper z-indexing and visual treatment with box-shadow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any browser compatibility CSS fixes
rg -g '*.scss' -A 3 'position:\s*sticky'
Length of output: 396
Script:
#!/bin/bash
# Check for browser prefixes and compatibility fixes in the pin mixin
cat packages/react-components/src/components/Table/styles/pin.scss
# Check for any browser compatibility comments or documentation
rg -g '*.{scss,css}' -B 2 -A 2 '@supports'
Length of output: 379
26-30: Consider RTL support for text alignment
The text-align: start is good for RTL support, but ensure the Design System has RTL testing in place.
✅ Verification successful
RTL support looks good with text-align: start
Using text-align: start is the correct approach for RTL support as it automatically handles text direction based on the document's direction. No additional RTL-specific styles are needed at the component level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for RTL related styles or utilities
rg -g '*.scss' 'rtl:|dir=rtl|[^-]right-to-left'
Length of output: 49
Script:
#!/bin/bash
# Let's check for any RTL-related configurations or utilities in the codebase
rg -i "rtl|right.?to.?left|direction|text-align|dir=" --type css --type scss --type ts --type tsx
# Also check for any RTL testing setup
fd -e test.js -e test.tsx -e spec.js -e spec.tsx -X rg -i "rtl|right.?to.?left|direction"
# Check package.json for RTL-related dependencies or scripts
fd "package.json" -X rg -i "rtl|direction"
Length of output: 263
Script:
#!/bin/bash
# Search for RTL related content with correct file type flags
rg -i "rtl|right.?to.?left|direction|text-align|dir=" -t css -t js -t jsx -t ts -t tsx
# Look specifically for text-align properties in style files
rg "text-align" -t css -t js -t jsx -t ts -t tsx -A 1 -B 1
Length of output: 203
51-59: Ensure sufficient color contrast for accessibility
Verify that the background colors provide sufficient contrast with text colors.
packages/react-components/src/components/Table/hooks.ts (2)
3-15: Well-structured TypeScript interfaces!
The interfaces are well-defined with proper generics and optional properties, providing good type safety and flexibility.
17-57: Clean and well-structured hook implementation!
The hook follows React best practices with proper memoization and clear separation of concerns.
packages/react-components/src/index.ts (1)
49-49: LGTM! Export follows established patterns.
The Table component export maintains consistency with other components and is correctly placed in alphabetical order.
✅ Verification successful
✅ Table component implementation verified
The Table component and its subcomponents are properly implemented at the expected location with all necessary files present.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Table component implementation exists
# Expected: Find the Table component implementation files
fd -t f "Table" packages/react-components/src/components/Table/
Length of output: 508
packages/react-components/src/components/Table/TableHeader.tsx (2)
1-20: LGTM! Well-organized imports and consistent BEM naming.
The code follows best practices for import organization and CSS class naming conventions.
50-50: Add type guard for column.key to String conversion.
The current implementation assumes column.key can be safely converted to string.
| position: sticky; | ||
| top: 0; | ||
| z-index: 5; | ||
| box-shadow: 0 12px 12px -7px #1313172e; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use design system color tokens for consistency
Replace the hardcoded shadow color with a design system token.
- box-shadow: 0 12px 12px -7px #1313172e;
+ box-shadow: 0 12px 12px -7px var(--shadow-color);Committable suggestion skipped: line range outside the PR's diff.
| export interface ITableProps<T> { | ||
| data: T[]; | ||
| columns: Column<T>[]; | ||
| stripped?: boolean; | ||
| size?: TableSize; | ||
| pin?: PinOptions; | ||
| getRowId: (item: T) => string | number; | ||
| resizable?: boolean; | ||
| selectable?: true; | ||
| selectedRows?: Set<string | number>; | ||
| onSelectionChange?: (selectedIds: Set<string | number>) => void; | ||
| } |
There was a problem hiding this comment.
Fix selectable prop type and add conditional requirements
The selectable prop should be boolean, and selectedRows should be required when selectable is true.
export interface ITableProps<T> {
data: T[];
columns: Column<T>[];
stripped?: boolean;
size?: TableSize;
pin?: PinOptions;
getRowId: (item: T) => string | number;
resizable?: boolean;
- selectable?: true;
+ selectable?: boolean;
- selectedRows?: Set<string | number>;
- onSelectionChange?: (selectedIds: Set<string | number>) => void;
+ selectedRows: selectable extends true ? Set<string | number> : never;
+ onSelectionChange: selectable extends true ? (selectedIds: Set<string | number>) => void : never;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface ITableProps<T> { | |
| data: T[]; | |
| columns: Column<T>[]; | |
| stripped?: boolean; | |
| size?: TableSize; | |
| pin?: PinOptions; | |
| getRowId: (item: T) => string | number; | |
| resizable?: boolean; | |
| selectable?: true; | |
| selectedRows?: Set<string | number>; | |
| onSelectionChange?: (selectedIds: Set<string | number>) => void; | |
| } | |
| export interface ITableProps<T> { | |
| data: T[]; | |
| columns: Column<T>[]; | |
| stripped?: boolean; | |
| size?: TableSize; | |
| pin?: PinOptions; | |
| getRowId: (item: T) => string | number; | |
| resizable?: boolean; | |
| selectable?: boolean; | |
| selectedRows: selectable extends true ? Set<string | number> : never; | |
| onSelectionChange: selectable extends true ? (selectedIds: Set<string | number>) => void : never; | |
| } |
| {data.map((row) => ( | ||
| <TableRow | ||
| key={getRowId(row)} | ||
| row={row} | ||
| columns={columns} | ||
| columnWidths={columnWidths} | ||
| selectable={selectable} | ||
| isSelected={isSelected} | ||
| toggleRowSelection={toggleRowSelection} | ||
| rowId={getRowId(row)} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize row rendering and add error handling.
- Cache row ID to avoid double calculation
- Consider adding error boundary and empty state handling
- Add virtualization for large datasets
- {data.map((row) => (
+ {data.map((row) => {
+ const rowId = getRowId(row);
+ return (
<TableRow
- key={getRowId(row)}
+ key={rowId}
row={row}
columns={columns}
columnWidths={columnWidths}
selectable={selectable}
isSelected={isSelected}
toggleRowSelection={toggleRowSelection}
- rowId={getRowId(row)}
+ rowId={rowId}
/>
- ))}
+ })}Consider using react-window or react-virtualized for handling large datasets efficiently.
Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <tbody> | ||
| {data.map((row) => ( | ||
| <TableRow | ||
| key={getRowId(row)} | ||
| row={row} | ||
| columns={columns} | ||
| columnWidths={columnWidths} | ||
| selectable={selectable} | ||
| isSelected={isSelected} | ||
| toggleRowSelection={toggleRowSelection} | ||
| rowId={getRowId(row)} | ||
| /> | ||
| ))} | ||
| </tbody> | ||
| ); |
There was a problem hiding this comment.
💡 Codebase verification
Add ARIA roles and test coverage for table components
- Add
role="rowgroup"to<tbody>element - Add
role="row"to<tr>elements in TableRow and TableHeader - Create test file
TableBody.test.tsxto cover row rendering and selection logic
🔗 Analysis chain
Add accessibility attributes and ensure test coverage.
- Add ARIA roles and attributes for better screen reader support
- Ensure test coverage for selection and rendering logic
Let me search for test coverage and accessibility patterns in a different way to make a more informed decision.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test file existence and accessibility attributes in related components
fd "TableBody.test.tsx" && \
rg -l "role=\"" packages/react-components/src/components/Table/
Length of output: 88
Script:
#!/bin/bash
# Check for test files and accessibility patterns in Table components
fd "Table.*test" && \
ast-grep --pattern 'role=' && \
rg -A 2 '<tbody' packages/react-components/src/components/Table/ && \
rg -A 2 '<tr' packages/react-components/src/components/Table/
Length of output: 981
| {selectable && ( | ||
| <td> | ||
| <Checkbox | ||
| checked={isSelected(rowId)} | ||
| onChange={() => toggleRowSelection(rowId)} | ||
| /> | ||
| </td> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance selection accessibility
Add keyboard navigation and ARIA attributes for better accessibility support.
{selectable && (
- <td>
+ <td
+ role="cell"
+ aria-selected={isSelected(rowId)}
+ >
<Checkbox
checked={isSelected(rowId)}
onChange={() => toggleRowSelection(rowId)}
+ aria-label={`Select row ${rowId}`}
/>
</td>
)}Committable suggestion skipped: line range outside the PR's diff.
| {columns.map((column, index) => ( | ||
| <td | ||
| key={String(column.key)} | ||
| style={{ width: `${columnWidths[index]}px` }} | ||
| > | ||
| {row[column.key] as React.ReactNode} | ||
| </td> | ||
| ))} |
There was a problem hiding this comment.
Add safety checks and improve accessibility
The current implementation has several potential issues:
- No handling for undefined column widths
- Unsafe type casting
- Missing accessibility attributes
Apply these improvements:
{columns.map((column, index) => (
<td
key={String(column.key)}
- style={{ width: `${columnWidths[index]}px` }}
+ style={{ width: columnWidths[index] ? `${columnWidths[index]}px` : 'auto' }}
+ role="cell"
+ data-column={String(column.key)}
>
- {row[column.key] as React.ReactNode}
+ {column.render ? column.render(row) : String(row[column.key] ?? '')}
</td>
))}Committable suggestion skipped: line range outside the PR's diff.
| &:hover, | ||
| &:active { | ||
| background-color: var(--action-primary-default); | ||
| height: 100vh; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider resizer hover height
Setting height to 100vh on hover could interfere with other page elements. Consider using a fixed height or percentage of the table height.
- height: 100vh;
+ height: 100%;
+ transform: scaleY(1.5);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &:hover, | |
| &:active { | |
| background-color: var(--action-primary-default); | |
| height: 100vh; | |
| } | |
| &:hover, | |
| &:active { | |
| background-color: var(--action-primary-default); | |
| height: 100%; | |
| transform: scaleY(1.5); | |
| } |
| const toggleRowSelection = useCallback( | ||
| (id: string | number) => { | ||
| const updatedSelectedRows = new Set(selectedRows); | ||
| if (updatedSelectedRows.has(id)) { | ||
| updatedSelectedRows.delete(id); | ||
| } else { | ||
| updatedSelectedRows.add(id); | ||
| } | ||
| onSelectionChange?.(updatedSelectedRows); | ||
| }, | ||
| [selectedRows, onSelectionChange] | ||
| ); |
There was a problem hiding this comment.
Fix potential runtime error in toggleRowSelection
The function assumes selectedRows exists when creating updatedSelectedRows, but it's an optional prop.
Apply this fix:
const toggleRowSelection = useCallback(
(id: string | number) => {
- const updatedSelectedRows = new Set(selectedRows);
+ const updatedSelectedRows = new Set(selectedRows || []);
if (updatedSelectedRows.has(id)) {
updatedSelectedRows.delete(id);
} else {
updatedSelectedRows.add(id);
}
onSelectionChange?.(updatedSelectedRows);
},
[selectedRows, onSelectionChange]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const toggleRowSelection = useCallback( | |
| (id: string | number) => { | |
| const updatedSelectedRows = new Set(selectedRows); | |
| if (updatedSelectedRows.has(id)) { | |
| updatedSelectedRows.delete(id); | |
| } else { | |
| updatedSelectedRows.add(id); | |
| } | |
| onSelectionChange?.(updatedSelectedRows); | |
| }, | |
| [selectedRows, onSelectionChange] | |
| ); | |
| const toggleRowSelection = useCallback( | |
| (id: string | number) => { | |
| const updatedSelectedRows = new Set(selectedRows || []); | |
| if (updatedSelectedRows.has(id)) { | |
| updatedSelectedRows.delete(id); | |
| } else { | |
| updatedSelectedRows.add(id); | |
| } | |
| onSelectionChange?.(updatedSelectedRows); | |
| }, | |
| [selectedRows, onSelectionChange] | |
| ); |
| <th | ||
| key={String(column.key)} | ||
| ref={(el) => { | ||
| if (columnRefs.current) { | ||
| columnRefs.current[index] = el; | ||
| } | ||
| }} | ||
| className={cx({ | ||
| [styles[`${headerCellClass}--hoverDisabled`]]: !column.sortable, | ||
| })} | ||
| onMouseEnter={() => setHoveredColumnIndex(index)} | ||
| onMouseLeave={() => setHoveredColumnIndex(null)} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for better accessibility.
The table header cells should have proper ARIA attributes for sorting.
<th
key={String(column.key)}
ref={(el) => {
if (columnRefs.current) {
columnRefs.current[index] = el;
}
}}
+ role="columnheader"
+ aria-sort={
+ sortConfig.key === column.key
+ ? sortConfig.direction.toLowerCase()
+ : undefined
+ }
className={cx({
[styles[`${headerCellClass}--hoverDisabled`]]: !column.sortable,
})}
onMouseEnter={() => setHoveredColumnIndex(index)}
onMouseLeave={() => setHoveredColumnIndex(null)}
>Committable suggestion skipped: line range outside the PR's diff.
| return [...data].sort((a, b) => { | ||
| const sortValueA = column.sortValue | ||
| ? column.sortValue(a) | ||
| : a[sortConfig.key as unknown as keyof T]; | ||
| const sortValueB = column.sortValue | ||
| ? column.sortValue(b) | ||
| : b[sortConfig.key as unknown as keyof T]; | ||
|
|
||
| if (sortValueA < sortValueB) { | ||
| return sortConfig.direction === SortOrder.Ascending ? -1 : 1; | ||
| } | ||
| if (sortValueA > sortValueB) { | ||
| return sortConfig.direction === SortOrder.Ascending ? 1 : -1; | ||
| } | ||
|
|
||
| return 0; | ||
| }); |
There was a problem hiding this comment.
Handle undefined or null values during sorting.
Ensure sortValueA and sortValueB are handled when they are undefined or null to prevent runtime errors.
Apply this diff to enhance robustness:
return [...data].sort((a, b) => {
const sortValueA = column.sortValue
? column.sortValue(a)
: a[sortConfig.key as unknown as keyof T];
const sortValueB = column.sortValue
? column.sortValue(b)
: b[sortConfig.key as unknown as keyof T];
+ if (sortValueA == null) return 1;
+ if (sortValueB == null) return -1;
if (sortValueA < sortValueB) {
return sortConfig.direction === SortOrder.Ascending ? -1 : 1;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
packages/react-components/src/components/Table/types.ts (2)
10-10: Consider expanding PinOptionsCurrent implementation only supports header pinning. Consider adding 'footer' and 'column' options for future use cases.
66-69: Strengthen type safety for sort configurationConsider constraining the key type to only sortable columns.
export interface SortConfig<T> { - key: keyof T | null; + key: Extract<keyof T, Column<T>['key']> | null; direction: SortOrder; }packages/react-components/src/components/Table/Table.mdx (6)
7-7: Add proper heading structure for navigationReplace the plain text navigation with proper heading structure for better accessibility.
-[Intro](#Intro) | [Component API](#ComponentAPI) | [Content Spec](#ContentSpec) +## Navigation +- [Intro](#Intro) +- [Component API](#ComponentAPI) +- [Content Spec](#ContentSpec)
20-26: Add type documentation and validationThe Data type would benefit from JSDoc comments and validation constraints for each field.
+/** + * Represents a row in the table + * @typedef {Object} Data + */ type Data = { + /** Unique identifier for the row */ id: number; + /** Full name of the person */ name: string; + /** Age in years (must be positive) */ age: number; + /** Job role or position */ role: string; + /** Action button or component */ action: React.ReactNode; };
51-57: Simplify action column configurationThe action column has redundant properties. The
sortValueis unnecessary whensortableis false.{ key: 'action', header: 'Action', - sortable: false, - sortValue: undefined, },
59-88: Enhance sample data varietyConsider diversifying the sample data to demonstrate:
- Different action button states (disabled, loading)
- Empty or null values
- Long text handling
92-95: Expand usage examplesConsider adding examples demonstrating:
- Row selection
- Column resizing
- Custom sorting
- Different size variants
103-109: Add context for Figma documentation linkProvide a brief description of what users will find in the Figma documentation (visual specs, design tokens, etc.).
+<p>View detailed visual specifications, measurements, and design tokens in our Figma documentation.</p> <a className="sb-unstyled" href="https://www.figma.com/design/9FDwjR8lYvincseDkKypC4/Component-Documentations?node-id=21446-18742&node-type=canvas&t=XufCAXr9dcNjfXDk-0" target="_blank" > Go to Figma documentation </a>packages/react-components/src/components/Table/Table.stories.tsx (3)
16-22: Add JSDoc documentation for the Data type.Adding documentation will help other developers understand the purpose and usage of each field in the Data type.
+/** + * Represents a row in the table with basic user information and an action button. + */ type Data = { + /** Unique identifier for the row */ id: number; + /** User's full name */ name: string; + /** User's age in years */ age: number; + /** User's role in the organization */ role: string; + /** Action button or other interactive element */ action: React.ReactNode; };
85-304: Extract repeated getRowId function.The
getRowIdfunction is duplicated across all stories. Consider moving it to a shared location.+const getRowId = (row: Data) => row.id; + export const Default: StoryFn = () => { - const getRowId = (row: Data) => row.id; return ( <Table size="medium" data={data} columns={columns} getRowId={getRowId} /> ); };
11-14: Add accessibility testing stories.Consider adding stories that demonstrate and test accessibility features such as keyboard navigation, screen reader support, and ARIA attributes.
Would you like me to help create accessibility testing stories for the Table component?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/react-components/src/components/Table/Table.mdx(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)
🔇 Additional comments (2)
packages/react-components/src/components/Table/types.ts (2)
12-16: LGTM!
The enum values are well-defined and follow conventional naming.
54-63: Fix selectable prop type and add conditional requirements
The selectable prop should be boolean, and selectedRows should be required when selectable is true.
| export type Column<T> = { | ||
| key: keyof T; | ||
| header: string; | ||
| sortable?: boolean; | ||
| sortValue?: (item: T) => string | number; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type constraint for sortable columns
When sortable is true, sortValue should be required to ensure proper sorting implementation.
export type Column<T> = {
key: keyof T;
header: string;
sortable?: boolean;
- sortValue?: (item: T) => string | number;
+ sortValue?: sortable extends true ? (item: T) => string | number : never;
};Committable suggestion skipped: line range outside the PR's diff.
| }, | ||
| { | ||
| id: 7, | ||
| name: 'Sam Green', | ||
| age: 45, | ||
| role: 'Manager', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 8, | ||
| name: 'Emily Clark', | ||
| age: 22, | ||
| role: 'Intern', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 9, | ||
| name: 'Chris Adams', | ||
| age: 36, | ||
| role: 'Developer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 10, | ||
| name: 'Sophia Turner', | ||
| age: 29, | ||
| role: 'Designer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 11, | ||
| name: 'James Taylor', | ||
| age: 39, | ||
| role: 'Manager', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 12, | ||
| name: 'Olivia Lewis', | ||
| age: 24, | ||
| role: 'Intern', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 13, | ||
| name: 'Daniel Evans', | ||
| age: 32, | ||
| role: 'Developer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 14, | ||
| name: 'Mia Harris', | ||
| age: 31, | ||
| role: 'Designer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 15, | ||
| name: 'Noah King', | ||
| age: 40, | ||
| role: 'Manager', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 16, | ||
| name: 'Liam Scott', | ||
| age: 23, | ||
| role: 'Intern', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 17, | ||
| name: 'Ethan White', | ||
| age: 26, | ||
| role: 'Developer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 18, | ||
| name: 'Emma Hall', | ||
| age: 35, | ||
| role: 'Designer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 19, | ||
| name: 'Benjamin Young', | ||
| age: 41, | ||
| role: 'Manager', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 20, | ||
| name: 'Ava Martinez', | ||
| age: 21, | ||
| role: 'Intern', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate data definition.
The dataForPinnExample is largely a duplicate of the data array. Consider creating a helper function to generate test data.
+const generateTestData = (count: number): Data[] => {
+ const roles = ['Developer', 'Designer', 'Manager', 'Intern'];
+ return Array.from({ length: count }, (_, i) => ({
+ id: i + 1,
+ name: `User ${i + 1}`,
+ age: 20 + Math.floor(Math.random() * 30),
+ role: roles[i % roles.length],
+ action: <Button>Edit</Button>,
+ }));
+};
+
-const data: Data[] = [/* ... */];
-const dataForPinnExample: Data[] = [/* ... */];
+const data = generateTestData(4);
+const dataForPinnExample = generateTestData(20);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dataForPinnExample: Data[] = [ | |
| { | |
| id: 1, | |
| name: 'John Doe', | |
| age: 28, | |
| role: 'Developer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 2, | |
| name: 'Jane Smith', | |
| age: 34, | |
| role: 'Designer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 3, | |
| name: 'Alice Johnson', | |
| age: 42, | |
| role: 'Manager', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 4, | |
| name: 'Mike Williams', | |
| age: 25, | |
| role: 'Intern', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 5, | |
| name: 'David Brown', | |
| age: 30, | |
| role: 'Developer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 6, | |
| name: 'Laura Wilson', | |
| age: 27, | |
| role: 'Designer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 7, | |
| name: 'Sam Green', | |
| age: 45, | |
| role: 'Manager', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 8, | |
| name: 'Emily Clark', | |
| age: 22, | |
| role: 'Intern', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 9, | |
| name: 'Chris Adams', | |
| age: 36, | |
| role: 'Developer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 10, | |
| name: 'Sophia Turner', | |
| age: 29, | |
| role: 'Designer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 11, | |
| name: 'James Taylor', | |
| age: 39, | |
| role: 'Manager', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 12, | |
| name: 'Olivia Lewis', | |
| age: 24, | |
| role: 'Intern', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 13, | |
| name: 'Daniel Evans', | |
| age: 32, | |
| role: 'Developer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 14, | |
| name: 'Mia Harris', | |
| age: 31, | |
| role: 'Designer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 15, | |
| name: 'Noah King', | |
| age: 40, | |
| role: 'Manager', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 16, | |
| name: 'Liam Scott', | |
| age: 23, | |
| role: 'Intern', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 17, | |
| name: 'Ethan White', | |
| age: 26, | |
| role: 'Developer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 18, | |
| name: 'Emma Hall', | |
| age: 35, | |
| role: 'Designer', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 19, | |
| name: 'Benjamin Young', | |
| age: 41, | |
| role: 'Manager', | |
| action: <Button>Edit</Button>, | |
| }, | |
| { | |
| id: 20, | |
| name: 'Ava Martinez', | |
| age: 21, | |
| role: 'Intern', | |
| action: <Button>Edit</Button>, | |
| }, | |
| ]; | |
| const generateTestData = (count: number): Data[] => { | |
| const roles = ['Developer', 'Designer', 'Manager', 'Intern']; | |
| return Array.from({ length: count }, (_, i) => ({ | |
| id: i + 1, | |
| name: `User ${i + 1}`, | |
| age: 20 + Math.floor(Math.random() * 30), | |
| role: roles[i % roles.length], | |
| action: <Button>Edit</Button>, | |
| })); | |
| }; | |
| const data = generateTestData(4); | |
| const dataForPinnExample = generateTestData(20); |
|
Great work!
|
|
Hover on sorting icon in some columns make shifts Monosnap.screencast.2024-11-27.10-39-53.mp4 |
|
Resizing affects both sides instead of only the side being adjusted, and it also triggers changes in the widths of other columns. resize.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/react-components/src/components/Table/Table.spec.tsx (1)
13-24: Enhance test data coverageWhile the current test data is good for basic cases, consider adding edge cases:
- Empty strings
- Very long text content
- Special characters
- Extreme number values
| describe('<Table> component', () => { | ||
| const getRowId = (row: Data) => row.id; | ||
|
|
||
| it('should render a table with provided data and columns', () => { | ||
| const { getByRole, getAllByRole } = render( | ||
| <Table data={data} columns={columns} getRowId={getRowId} /> | ||
| ); | ||
|
|
||
| const table = getByRole('table'); | ||
| expect(table).toBeInTheDocument(); | ||
|
|
||
| const rows = getAllByRole('row'); | ||
| expect(rows.length).toBe(data.length + 1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for table dimensions
Based on the PR objectives, we need tests to verify:
- Header height (36px)
- Cell sizes (Small: 48px, Medium: 56px, Large: 64px)
There was a problem hiding this comment.
We verify styling using Chromatic, and our eyes :) Not needed to cover it by the test
There was a problem hiding this comment.
@marcinsawicki Understood. We'll rely on Chromatic and visual inspection for styling verification; tests for styling dimensions in the Table component are not necessary.
✏️ Learnings added
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.spec.tsx:26-40
Timestamp: 2024-12-04T07:42:16.153Z
Learning: In the Design System project, for the `Table` component implemented in React, styling is verified using Chromatic and visual inspection; tests for styling dimensions such as header height and cell sizes are not necessary in `Table.spec.tsx`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| it('should allow row selection if selectable prop is enabled', () => { | ||
| const onSelectionChange = vi.fn(); | ||
| const { getAllByRole } = render( | ||
| <Table | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| selectable={true} | ||
| onSelectionChange={onSelectionChange} | ||
| /> | ||
| ); | ||
|
|
||
| const checkboxes = getAllByRole('checkbox'); | ||
| expect(checkboxes.length).toBe(data.length + 1); | ||
|
|
||
| fireEvent.click(checkboxes[1]); | ||
| expect(onSelectionChange).toHaveBeenCalledWith(new Set([1])); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for remaining table features
The selection test is well-implemented, but we're missing tests for:
- Column resizing functionality
- Pinned headers with shadows
- Spacing between label and sorting icon (8px)
- Text styles (paragraph-md, paragraph-sm-bold)
| import { render, fireEvent, vi } from 'test-utils'; | ||
|
|
||
| import { Table } from './Table'; | ||
| import { Column } from './types'; | ||
|
|
||
| type Data = { | ||
| id: number; | ||
| name: string; | ||
| age: number; | ||
| role: string; | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Add test coverage for sorting functionality
The Table component has robust sorting implementation with:
- Column-level sort configuration
- Custom sort value functions
- Multi-direction sorting (Ascending/Descending/None)
But the test file lacks coverage for these features. Add tests for:
handleSortfunctionality when clicking column headers- Sort direction cycling
- Custom
sortValuefunction behavior - Verify sorted data order
🔗 Analysis chain
Add test coverage for sortable columns
The Data type and imports are well-structured, but given that all columns are marked as sortable (lines 14-17), we should add tests for sorting functionality.
Consider adding these test cases:
- Clicking column header changes sort direction
- Verify sort order for different data types (string, number)
- Test multiple column sort if supported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if sorting tests exist in other test files
rg -l "sort.*test|test.*sort" --type ts --type tsx
Length of output: 80
Script:
#!/bin/bash
# Let's try a different approach to find sorting-related tests
rg -l "sort" --type typescript --type-add 'typescript:*.{ts,tsx}'
# Also check the Table component implementation for sortable props
ast-grep --pattern 'interface $_ {
$$$
sortable?: $_
$$$
}'
# And check for sort-related functions
ast-grep --pattern 'function $_($$_) {
$$$
sort$$$
}'
Length of output: 954
Script:
#!/bin/bash
# Let's check the actual content of Table-related files for sort functionality
rg "sort" -C 3 packages/react-components/src/components/Table/types.ts
rg "sort" -C 3 packages/react-components/src/components/Table/Table.tsx
rg "test|it\(" -C 3 packages/react-components/src/components/Table/Table.spec.tsx
Length of output: 2905
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/react-components/src/components/Table/TableRow.tsx (1)
12-20: Consider using a more specific type for rowIdThe
rowIdprop accepts both string and number, which could lead to inconsistent handling. Consider using a single type or creating a specific ID type.- rowId: string | number; + rowId: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/react-components/src/components/Table/TableHeader.module.scss(1 hunks)packages/react-components/src/components/Table/TableHeader.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.tsx(1 hunks)packages/react-components/src/components/Table/styles/size.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-components/src/components/Table/TableHeader.module.scss
- packages/react-components/src/components/Table/TableHeader.tsx
- packages/react-components/src/components/Table/styles/size.scss
🔇 Additional comments (1)
packages/react-components/src/components/Table/TableRow.tsx (1)
33-40: Enhance selection accessibility
The checkbox implementation needs accessibility improvements.
| {columns.map((column, index) => ( | ||
| <td | ||
| key={String(column.key)} | ||
| style={{ width: `${columnWidths[index]}px` }} | ||
| > | ||
| <Text size="md" noMargin> | ||
| {row[column.key] as React.ReactNode} | ||
| </Text> | ||
| </td> | ||
| ))} |
There was a problem hiding this comment.
Add width fallback and improve type safety
The current implementation needs improvements:
- Handle undefined column widths gracefully
- Add type-safe cell rendering
{columns.map((column, index) => (
<td
key={String(column.key)}
- style={{ width: `${columnWidths[index]}px` }}
+ style={{ width: columnWidths[index] ? `${columnWidths[index]}px` : 'auto' }}
>
<Text size="md" noMargin>
- {row[column.key] as React.ReactNode}
+ {column.render?.(row[column.key]) ?? String(row[column.key] ?? '')}
</Text>
</td>
))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {columns.map((column, index) => ( | |
| <td | |
| key={String(column.key)} | |
| style={{ width: `${columnWidths[index]}px` }} | |
| > | |
| <Text size="md" noMargin> | |
| {row[column.key] as React.ReactNode} | |
| </Text> | |
| </td> | |
| ))} | |
| {columns.map((column, index) => ( | |
| <td | |
| key={String(column.key)} | |
| style={{ width: columnWidths[index] ? `${columnWidths[index]}px` : 'auto' }} | |
| > | |
| <Text size="md" noMargin> | |
| {column.render?.(row[column.key]) ?? String(row[column.key] ?? '')} | |
| </Text> | |
| </td> | |
| ))} |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)
🔇 Additional comments (1)
packages/react-components/src/components/Table/Table.stories.tsx (1)
54-83: 🛠️ Refactor suggestion
Standardize button sizes and refactor duplicate data
- The
Buttonsize prop is inconsistent between data arrays ('xcompact' vs default) - Data arrays contain duplicate structure
+const generateTestData = (count: number, buttonSize: string = 'xcompact'): Data[] => {
+ const roles = ['Developer', 'Designer', 'Manager', 'Intern'];
+ return Array.from({ length: count }, (_, i) => ({
+ id: i + 1,
+ name: `User ${i + 1}`,
+ age: 20 + Math.floor(Math.random() * 30),
+ role: roles[i % roles.length],
+ action: <Button size={buttonSize}>Edit</Button>,
+ }));
+};
-const data: Data[] = [/* ... */];
-const dataForPinnExample: Data[] = [/* ... */];
+const data = generateTestData(4);
+const dataForPinnExample = generateTestData(20);Also applies to: 130-271
| const columns: Column<(typeof data)[0]>[] = [ | ||
| { | ||
| key: 'id', | ||
| header: 'ID', | ||
| sortable: true, | ||
| }, | ||
| { | ||
| key: 'name', | ||
| header: 'Name', | ||
| sortable: true, | ||
| sortValue: (item: Data) => item.name.toLowerCase(), | ||
| }, | ||
| { | ||
| key: 'age', | ||
| header: 'Age', | ||
| sortable: true, | ||
| }, | ||
| { | ||
| key: 'role', | ||
| header: 'Role', | ||
| sortable: true, | ||
| sortValue: (item: Data) => item.role, | ||
| }, | ||
| { | ||
| key: 'action', | ||
| header: 'Action', | ||
| sortable: false, | ||
| sortValue: undefined, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align header specifications with design requirements
According to the design requirements:
- Header height should be 48px
- Header font should use paragraph-sm-bold
- Add 8px spacing between label and sorting icon
| role: 'Manager', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 12, | ||
| name: 'Olivia Lewis', | ||
| age: 24, | ||
| role: 'Intern', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 13, | ||
| name: 'Daniel Evans', | ||
| age: 32, | ||
| role: 'Developer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 14, | ||
| name: 'Mia Harris', | ||
| age: 31, | ||
| role: 'Designer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 15, | ||
| name: 'Noah King', | ||
| age: 40, | ||
| role: 'Manager', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 16, | ||
| name: 'Liam Scott', | ||
| age: 23, | ||
| role: 'Intern', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 17, | ||
| name: 'Ethan White', | ||
| age: 26, | ||
| role: 'Developer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 18, | ||
| name: 'Emma Hall', | ||
| age: 35, | ||
| role: 'Designer', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 19, | ||
| name: 'Benjamin Young', | ||
| age: 41, | ||
| role: 'Manager', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| { | ||
| id: 20, | ||
| name: 'Ava Martinez', | ||
| age: 21, | ||
| role: 'Intern', | ||
| action: <Button>Edit</Button>, | ||
| }, | ||
| ]; | ||
|
|
||
| export const Pinning: StoryFn = () => { | ||
| const getRowId = (row: Data) => row.id; | ||
|
|
||
| return ( | ||
| <Table | ||
| size="medium" | ||
| data={dataForPinnExample} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| pin="header" | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| export const Selecting: StoryFn = () => { | ||
| const getRowId = (row: Data) => row.id; | ||
| const [selectedRows, setSelectedRows] = React.useState<Set<string | number>>( | ||
| new Set() | ||
| ); | ||
|
|
||
| return ( | ||
| <Table | ||
| size="medium" | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| selectable | ||
| selectedRows={selectedRows} | ||
| onSelectionChange={setSelectedRows} | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement standardized cell sizes
According to design requirements, cell heights should be:
- Small: 48px
- Medium: 56px
- Large: 64px
Current implementation doesn't enforce these sizes.
| export const Pinning: StoryFn = () => { | ||
| const getRowId = (row: Data) => row.id; | ||
|
|
||
| return ( | ||
| <Table | ||
| size="medium" | ||
| data={dataForPinnExample} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| pin="header" | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add shadow tokens for pinned elements
Add the following shadow tokens:
--shadow-fixed-bottomfor pinned header--shadow-fixed-rightfor left column--shadow-fixed-leftfor right column
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/react-components/src/components/Table/Table.spec.tsx(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-components/src/components/Table/Table.spec.tsx
- packages/react-components/src/components/Table/Table.stories.tsx
🔇 Additional comments (3)
packages/react-components/src/components/Table/Table.tsx (3)
127-143: Handle undefined or null values during sorting
The sorting logic needs to handle null/undefined values to prevent runtime errors.
22-22: Verify size prop default value
According to design requirements, table cells should follow specific heights:
- Small: 48px
- Medium: 56px
- Large: 64px
Please ensure these dimensions are properly implemented in the CSS.
✅ Verification successful
Size prop default value aligns with design requirements
The CSS implementation in Table.module.scss and styles/size.scss correctly implements the required dimensions:
- Small: 48px (default)
- Medium: 56px
- Large: 64px
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the CSS implements the required dimensions
rg -A 2 "table--small|table--medium|table--large" packages/react-components/src/components/Table/Table.module.scss
Length of output: 116
Script:
#!/bin/bash
# Let's check if the file exists first and see its content
fd "Table.module.scss" --type f
# Check for any height-related styles in the table component
rg -i "height|px" packages/react-components/src/components/Table/
Length of output: 2265
184-194: Verify header styling implementation
Design requirements specify:
- Header height: 48px
- Font size: paragraph-sm-bold
- 8px spacing between label and sorting icon
Please ensure these are implemented in TableHeader component.
✅ Verification successful
Header styling requirements met
The implementation matches the design requirements:
- Header height: 48px (using
--spacing-12) - Font size: Using Text component with
size="sm"andboldprops - Icon spacing: 8px gap (using
--spacing-2in content class)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check TableHeader styling implementation
rg -A 3 "header|sort-icon" packages/react-components/src/components/Table/TableHeader.module.scss
Length of output: 198
Script:
#!/bin/bash
# Let's check the TableHeader component implementation for styling details
ast-grep --pattern 'const TableHeader = ($_) => {
$$$
}'
# Also check for any style imports and className usage
rg -l "TableHeader.module.scss" packages/react-components/src/components/Table/
rg "className.*header" packages/react-components/src/components/Table/TableHeader.tsx
# Check for design tokens or variables that might define these values
fd "tokens|variables|theme" --extension scss,css packages/react-components/src/
Length of output: 373
Script:
#!/bin/bash
# Let's check the actual content of TableHeader component and its styles
cat packages/react-components/src/components/Table/TableHeader.tsx
cat packages/react-components/src/components/Table/TableHeader.module.scss
# Check for any typography or spacing tokens that might be used
rg -A 2 "paragraph-sm-bold|--spacing" packages/react-components/src/
Length of output: 65184
| const handleMouseDown = (index: number, event: React.MouseEvent) => { | ||
| event.preventDefault(); | ||
| const startX = event.clientX; | ||
| const startWidth = | ||
| columnWidths[index] || (event.currentTarget as HTMLElement).offsetWidth; | ||
| const nextWidth = columnWidths[index + 1] || 0; | ||
|
|
||
| const SCALING_FACTOR = 1.05; | ||
|
|
||
| const handleMouseMove = (moveEvent: MouseEvent) => { | ||
| const delta = (moveEvent.clientX - startX) * SCALING_FACTOR; | ||
|
|
||
| setColumnWidths((prevWidths) => { | ||
| const newWidths = [...prevWidths]; | ||
| const newWidth = Math.max(50, startWidth + delta); | ||
| const adjustedNextWidth = Math.max(50, nextWidth - delta); | ||
|
|
||
| newWidths[index] = newWidth; | ||
|
|
||
| if (index + 1 < newWidths.length) { | ||
| newWidths[index + 1] = adjustedNextWidth; | ||
| } | ||
|
|
||
| return newWidths; | ||
| }); | ||
| }; | ||
|
|
||
| const handleMouseUp = () => { | ||
| document.removeEventListener('mousemove', handleMouseMove); | ||
| document.removeEventListener('mouseup', handleMouseUp); | ||
| }; | ||
|
|
||
| document.addEventListener('mousemove', handleMouseMove); | ||
| document.addEventListener('mouseup', handleMouseUp); | ||
| }; |
There was a problem hiding this comment.
Modify column resizing behavior
Current implementation affects both the resized column and its neighbor. According to feedback, resizing should only affect the column being adjusted.
Remove the adjacent column width adjustment:
setColumnWidths((prevWidths) => {
const newWidths = [...prevWidths];
const newWidth = Math.max(50, startWidth + delta);
- const adjustedNextWidth = Math.max(50, nextWidth - delta);
newWidths[index] = newWidth;
- if (index + 1 < newWidths.length) {
- newWidths[index + 1] = adjustedNextWidth;
- }
return newWidths;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleMouseDown = (index: number, event: React.MouseEvent) => { | |
| event.preventDefault(); | |
| const startX = event.clientX; | |
| const startWidth = | |
| columnWidths[index] || (event.currentTarget as HTMLElement).offsetWidth; | |
| const nextWidth = columnWidths[index + 1] || 0; | |
| const SCALING_FACTOR = 1.05; | |
| const handleMouseMove = (moveEvent: MouseEvent) => { | |
| const delta = (moveEvent.clientX - startX) * SCALING_FACTOR; | |
| setColumnWidths((prevWidths) => { | |
| const newWidths = [...prevWidths]; | |
| const newWidth = Math.max(50, startWidth + delta); | |
| const adjustedNextWidth = Math.max(50, nextWidth - delta); | |
| newWidths[index] = newWidth; | |
| if (index + 1 < newWidths.length) { | |
| newWidths[index + 1] = adjustedNextWidth; | |
| } | |
| return newWidths; | |
| }); | |
| }; | |
| const handleMouseUp = () => { | |
| document.removeEventListener('mousemove', handleMouseMove); | |
| document.removeEventListener('mouseup', handleMouseUp); | |
| }; | |
| document.addEventListener('mousemove', handleMouseMove); | |
| document.addEventListener('mouseup', handleMouseUp); | |
| }; | |
| const handleMouseDown = (index: number, event: React.MouseEvent) => { | |
| event.preventDefault(); | |
| const startX = event.clientX; | |
| const startWidth = | |
| columnWidths[index] || (event.currentTarget as HTMLElement).offsetWidth; | |
| const nextWidth = columnWidths[index + 1] || 0; | |
| const SCALING_FACTOR = 1.05; | |
| const handleMouseMove = (moveEvent: MouseEvent) => { | |
| const delta = (moveEvent.clientX - startX) * SCALING_FACTOR; | |
| setColumnWidths((prevWidths) => { | |
| const newWidths = [...prevWidths]; | |
| const newWidth = Math.max(50, startWidth + delta); | |
| newWidths[index] = newWidth; | |
| return newWidths; | |
| }); | |
| }; | |
| const handleMouseUp = () => { | |
| document.removeEventListener('mousemove', handleMouseMove); | |
| document.removeEventListener('mouseup', handleMouseUp); | |
| }; | |
| document.addEventListener('mousemove', handleMouseMove); | |
| document.addEventListener('mouseup', handleMouseUp); | |
| }; |
There was a problem hiding this comment.
Please verify this comment
There was a problem hiding this comment.
@marcinsawicki, I've verified the comment and the suggested code change is correct. Making this modification will ensure that column resizing only affects the column being adjusted, aligning with the expected behavior.
✏️ Learnings added
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.tsx:57-91
Timestamp: 2024-12-04T07:51:00.400Z
Learning: In the `Table` component, when implementing column resizing, ensure that resizing affects only the column being resized and does not adjust adjacent columns.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
It works much worse with this change
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
arturfracala
force-pushed
the
feature/DPS-4377
branch
from
2fcd0b6 to
70726be
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/react-components/src/components/Table/TableBody.tsx (1)
4-12: Consider strengthening columnWidths typeThe type could be more specific to ensure columnWidths array length matches columns array length.
- columnWidths: (number | undefined)[]; + columnWidths: { [K in keyof T[]]?: number };packages/react-components/src/components/Table/Table.mdx (1)
11-11: Document known limitations and behavior notes.Add notes about:
- Sorting icon spacing (8px between label and icon)
- Column resizing behavior (currently affects both sides)
packages/react-components/src/components/Table/hooks.ts (1)
55-55: DefaultselectedCountto zero when undefinedConsider defaulting
selectedCountto0to avoid potential undefined values.Apply this diff if you agree:
- selectedCount: selectedRows?.size, + selectedCount: selectedRows?.size ?? 0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
packages/react-components/src/components/Table/Table.mdx(1 hunks)packages/react-components/src/components/Table/Table.module.scss(1 hunks)packages/react-components/src/components/Table/Table.spec.tsx(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.module.scss(1 hunks)packages/react-components/src/components/Table/TableHeader.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.module.scss(1 hunks)packages/react-components/src/components/Table/TableRow.tsx(1 hunks)packages/react-components/src/components/Table/hooks.ts(1 hunks)packages/react-components/src/components/Table/index.ts(1 hunks)packages/react-components/src/components/Table/styles/pin.scss(1 hunks)packages/react-components/src/components/Table/styles/size.scss(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/react-components/src/components/Table/Table.module.scss
- packages/react-components/src/components/Table/Table.spec.tsx
- packages/react-components/src/components/Table/Table.stories.tsx
- packages/react-components/src/components/Table/Table.tsx
- packages/react-components/src/components/Table/TableHeader.module.scss
- packages/react-components/src/components/Table/TableHeader.tsx
- packages/react-components/src/components/Table/TableRow.module.scss
- packages/react-components/src/components/Table/TableRow.tsx
- packages/react-components/src/components/Table/index.ts
- packages/react-components/src/components/Table/styles/pin.scss
- packages/react-components/src/components/Table/styles/size.scss
- packages/react-components/src/components/Table/types.ts
- packages/react-components/src/index.ts
🔇 Additional comments (7)
packages/react-components/src/components/Table/TableBody.tsx (3)
1-3: LGTM!
Imports are clean and specific to the component's needs.
24-41: Consider performance optimization for large datasets
For tables with many rows, consider implementing virtualization to improve rendering performance.
Consider using react-window or react-virtualized libraries for efficient rendering of large datasets.
37-37:
Avoid redundant rowId calculation
The rowId is already calculated on line 26, reuse it instead of calling getRowId again.
- rowId={getRowId(row)}
+ rowId={rowId}packages/react-components/src/components/Table/Table.mdx (1)
1-8: LGTM! Clean imports and clear navigation structure.
packages/react-components/src/components/Table/hooks.ts (3)
23-26: Efficient memoization with useCallback
Great use of useCallback to memoize isSelected, optimizing rendering performance.
28-39: Robust handling of selectedRows in toggleRowSelection
Initializing updatedSelectedRows with selectedRows || [] ensures stability when selectedRows is undefined.
41-49: Well-implemented toggleSelectAll logic
The toggle functionality correctly handles selecting and deselecting all rows, accounting for current selection state.
|
|
||
| ## Intro <a id="Intro" /> | ||
|
|
||
| `Table` component is a flexible and interactive table designed for displaying structured data. It supports key features such as sorting, column resizing, row selection, and a customizable layout. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add specific sizing and styling specifications to the introduction.
Based on the PR requirements, include the following specifications:
- Header height: 48px
- Cell sizes: Small (48px), Medium (56px), Large (64px)
- Header font: paragraph-sm-bold
- Cell text style: paragraph-md
There was a problem hiding this comment.
@marcinsawicki Understood.
(_/)
( •_•)
✏️ Learnings added
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.mdx:11-11
Timestamp: 2024-12-04T07:31:14.203Z
Learning: When reviewing documentation files for the Table component, avoid suggesting adding specific sizing and styling specifications to the introduction unless explicitly requested.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| ```jsx | ||
|
|
||
| type Data = { | ||
| id: number; | ||
| name: string; | ||
| age: number; | ||
| role: string; | ||
| action: React.ReactNode; | ||
| }; | ||
|
|
||
| const columns: Column<(typeof data)[0]>[] = [ | ||
| { | ||
| key: 'id', | ||
| header: 'ID', | ||
| sortable: true, | ||
| sortValue: (item: Data) => item.id, | ||
| }, | ||
| { | ||
| key: 'name', | ||
| header: 'Name', | ||
| sortable: true, | ||
| sortValue: (item: Data) => item.name.toLowerCase(), | ||
| }, | ||
| { | ||
| key: 'age', | ||
| header: 'Age', | ||
| sortable: true, | ||
| sortValue: (item: Data) => item.age, | ||
| }, | ||
| { | ||
| key: 'role', | ||
| header: 'Role', | ||
| sortable: true, | ||
| sortValue: (item: Data) => item.role, | ||
| }, | ||
| { | ||
| key: 'action', | ||
| header: 'Action', | ||
| }, | ||
| ]; | ||
|
|
||
| const data: Data[] = [ | ||
| { | ||
| id: 1, | ||
| name: 'John Doe', | ||
| age: 28, | ||
| role: 'Developer', | ||
| action: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| { | ||
| id: 2, | ||
| name: 'Jane Smith', | ||
| age: 34, | ||
| role: 'Designer', | ||
| action: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| { | ||
| id: 3, | ||
| name: 'Alice Johnson', | ||
| age: 42, | ||
| role: 'Manager', | ||
| action: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| { | ||
| id: 4, | ||
| name: 'Mike Williams', | ||
| age: 25, | ||
| role: 'Intern', | ||
| action: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| ]; | ||
|
|
||
| const getRowId = (row: Data) => row.id; | ||
|
|
||
| return ( | ||
| <Table size="medium" data={data} columns={columns} getRowId={getRowId} /> | ||
| ); | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance example with all supported features.
The example should demonstrate additional key features:
- Column resizing
- Row selection
- Striped styling
- Pinned headers with shadows (using shadow tokens: --shadow-fixed-bottom, --shadow-fixed-right, --shadow-fixed-left)
|
|
||
| ## Component API <a id="ComponentAPI" /> | ||
|
|
||
| <ArgTypes of={Stories.Default} /> |
There was a problem hiding this comment.
💡 Codebase verification
Stories.Default doesn't fully document available props
The Default story only shows basic usage, while the component supports many features demonstrated in separate stories:
- Size options (small/medium/large) → shown in Sizes story
- Column resizing → shown in Resizing story
- Row selection → shown in Selecting story
- Pinning → shown in Pinning story
🔗 Analysis chain
Verify complete props documentation in Stories.Default.
Ensure all component props are properly documented in the story, including:
- Size options (small/medium/large)
- Column resizing props
- Row selection props
- Striped styling props
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all props are documented in the story file
ast-grep --pattern 'export const Default = {
args: {
$$$
}
}' packages/react-components/src/components/Table/Table.stories.tsx
Length of output: 136
Script:
#!/bin/bash
# Let's first check if the file exists and see its content
fd "Table.stories.tsx" --exec cat {}
# Also check the Table component itself for props
fd "Table.tsx" --type f --exec ast-grep --pattern 'interface TableProps {
$$$
}'
Length of output: 5683
Script:
#!/bin/bash
# Let's check the Table component interface
fd "Table.tsx" --exec cat {}
Length of output: 11344
There was a problem hiding this comment.
Please verify if the "Component API" table covers all props from the interface
There was a problem hiding this comment.
@marcinsawicki, the "Component API" table currently references Stories.Default, which doesn't include all props from the ITableProps interface. To ensure all props are documented, you can update the ArgTypes in Table.mdx to reference the Table component directly:
<ArgTypes of={Table} />This will display all the component's props in the API table.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and extract the ITableProps interface from types.ts
fd 'types.ts' --type f --exec ast-grep --json --pattern 'export interface ITableProps<T> {
$$$
}' {} \;
# Extract the prop names from the ITableProps interface
fd 'types.ts' --type f --exec ast-grep --json --pattern 'export interface ITableProps<T> {
$$$
}' {} \; | jq -r '.captures[0].body.members[].key.name'
# Find Table.stories.tsx and extract the args from the Default story
fd 'Table.stories.tsx' --type f --exec ast-grep --json --pattern 'export const Default = {
args: {
$$$
},
};' {} \;
# Extract the arg names from the Default story
fd 'Table.stories.tsx' --type f --exec ast-grep --json --pattern 'export const Default = {
args: {
$$$
},
};' {} \; | jq -r '.captures[0].args.properties[].key.name'
Length of output: 8354
🧠 Learnings used
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.mdx:11-11
Timestamp: 2024-12-04T07:31:14.364Z
Learning: When reviewing documentation files for the Table component, avoid suggesting adding specific sizing and styling specifications to the introduction unless explicitly requested.
There was a problem hiding this comment.
All props are covered.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
packages/react-components/src/components/Table/TableHeader.module.scss (1)
29-31: Avoid using !importantConsider increasing specificity instead of using !important for better maintainability.
- background-color: var(--background-01) !important; + background-color: var(--background-01);Then increase selector specificity:
&__cell--hoverDisabled#{&}:hover { background-color: var(--background-01); }packages/react-components/src/components/Table/types.ts (2)
12-12: Document future extensibility of PinOptionsConsider adding a comment indicating that PinOptions may support additional pinning options in the future.
+// Note: Currently only supports 'header'. Future versions may support column pinning. export type PinOptions = 'header';
79-82: Consider using undefined for unsorted stateSince we have
SortOrder.None, usingnullfor the unsorted state is redundant.export interface SortConfig<T> { - key: keyof T | null; + key: keyof T | undefined; direction: SortOrder; }packages/react-components/src/components/Table/Table.stories.tsx (1)
67-67: Standardize button sizes across examplesThe
Buttonsize prop is inconsistent:
xcompactin thedataarray- No size specified in
dataForPinnExampleStandardize the button size across all examples.
Also applies to: 74-74, 81-81, 88-88, 143-143, 150-150, 157-157, 164-164, 171-171, 178-178, 185-185, 192-192, 199-199, 206-206, 213-213, 220-220, 227-227, 234-234, 241-241, 248-248, 255-255, 262-262, 269-269, 276-276
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/react-components/src/components/Table/Table.module.scss(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.module.scss(1 hunks)packages/react-components/src/components/Table/TableHeader.tsx(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-components/src/components/Table/Table.tsx
- packages/react-components/src/components/Table/TableHeader.tsx
🔇 Additional comments (13)
packages/react-components/src/components/Table/TableHeader.module.scss (1)
49-53: Reconsider resizer hover height
Setting height to 100vh on hover could interfere with other page elements.
- height: 100vh;
+ height: 100%;
+ transform: scaleY(1.5);packages/react-components/src/components/Table/Table.module.scss (6)
1-6: LGTM! Clean modular SCSS setup
7-11: LGTM! Good performance optimization with contain: paint
12-22: LGTM! Size variants match design requirements
34-51: LGTM! Good use of design system spacing tokens
53-61: LGTM! Proper use of design system color tokens
77-95: LGTM! Well-structured action bar styles
packages/react-components/src/components/Table/types.ts (2)
3-8: Ensure type safety for sortable columns
When a column is marked as sortable, it should have a required sortValue function.
56-65: Improve type safety for selection props
The selectable prop should be boolean, and selection-related props should be conditionally required.
packages/react-components/src/components/Table/Table.stories.tsx (4)
30-59: Add header specifications to column definitions
The column definitions should include header specifications as per design requirements:
- Header height: 48px
- Font: paragraph-sm-bold
- Spacing: 8px between label and sorting icon
61-90: Eliminate data duplication
The dataForPinnExample duplicates the structure of data. Create a helper function to generate test data with configurable size.
Also applies to: 137-278
114-135: Implement standardized cell sizes
Update size variants to match design requirements:
- Small: 48px
- Medium: 56px
- Large: 64px
280-292: Add shadow tokens for pinned elements
Implement required shadow tokens for pinned elements:
--shadow-fixed-bottomfor pinned header--shadow-fixed-rightfor left column--shadow-fixed-leftfor right column
| .#{$base-class} { | ||
| border-bottom: 1px solid var(--border-basic-tertiary); | ||
| background-color: var(--background-01); | ||
| height: var(--spacing-12); |
There was a problem hiding this comment.
Align with design specifications
According to the design requirements:
- Header height should be 48px instead of var(--spacing-12)
- Font size should use paragraph-sm-bold token instead of 13px
- height: var(--spacing-12);
+ height: 48px;
- font-size: 13px;
+ font: var(--typography-paragraph-sm-bold);Also applies to: 12-12
| > span { | ||
| margin-left: var(--spacing-1); | ||
| } |
There was a problem hiding this comment.
Adjust icon spacing
The spacing between label and sorting icon should be 8px per design requirements.
- margin-left: var(--spacing-1);
+ margin-left: var(--spacing-2); /* 8px */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > span { | |
| margin-left: var(--spacing-1); | |
| } | |
| > span { | |
| margin-left: var(--spacing-2); /* 8px */ | |
| } |
| th > div { | ||
| display: flex; | ||
| } |
There was a problem hiding this comment.
Add spacing between label and sorting icon
According to design requirements, there should be 8px spacing between the label and sorting icon.
th > div {
display: flex;
+ gap: var(--spacing-1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| th > div { | |
| display: flex; | |
| } | |
| th > div { | |
| display: flex; | |
| gap: var(--spacing-1); | |
| } |
| &--pinned_header { | ||
| @include pin.generatePin('header'); | ||
| } |
There was a problem hiding this comment.
Add shadow token for pinned header
According to design requirements, pinned header should use --shadow-fixed-bottom token.
&--pinned_header {
@include pin.generatePin('header');
+ box-shadow: var(--shadow-fixed-bottom);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &--pinned_header { | |
| @include pin.generatePin('header'); | |
| } | |
| &--pinned_header { | |
| @include pin.generatePin('header'); | |
| box-shadow: var(--shadow-fixed-bottom); | |
| } |
There was a problem hiding this comment.
It uses --shadow-fixed-bottom. It generates from function pin.generatePin
There was a problem hiding this comment.
@arturfracala, thank you for the clarification. I see that @include pin.generatePin('header'); already includes the --shadow-fixed-bottom token. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: arturfracala
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.module.scss:25-27
Timestamp: 2024-12-09T08:35:31.503Z
Learning: `@include pin.generatePin('header');` already applies the `--shadow-fixed-bottom` shadow token in the SCSS styling.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| &__cell { | ||
| text-align: start; | ||
| line-height: 21px; | ||
| font-size: 14px; | ||
| } |
There was a problem hiding this comment.
Use design system tokens for typography
Replace hardcoded values with design system tokens:
&__cell {
text-align: start;
- line-height: 21px;
- font-size: 14px;
+ @extend .paragraph-md;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &__cell { | |
| text-align: start; | |
| line-height: 21px; | |
| font-size: 14px; | |
| } | |
| &__cell { | |
| text-align: start; | |
| @extend .paragraph-md; | |
| } |
There was a problem hiding this comment.
To verify if it is possible to use our typography in the cell instead styling it
There was a problem hiding this comment.
@marcinsawicki, thank you for verifying this. Using your typography in the cell instead of custom styling would enhance consistency with the design system.
✏️ Learnings added
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.module.scss:33-37
Timestamp: 2024-12-04T07:39:30.858Z
Learning: When styling table cells in the `Table` component, use design system typography classes instead of custom styles.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
arturfracala
force-pushed
the
feature/DPS-4377
branch
from
e163eab to
64a6396
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (9)
packages/react-components/src/components/Table/TableHeader.spec.tsx (1)
1-13: Add @testing-library/jest-dom for accessibility testingConsider importing
@testing-library/jest-domto enhance accessibility testing capabilities, especially for checking ARIA attributes and roles in the table header.+import '@testing-library/jest-dom';packages/react-components/src/components/Table/TableRow.spec.tsx (1)
6-21: Consider extracting test data to a separate fileThe test data (types, columns, row) could be reused across other table component test files.
Create a new file
__tests__/tableTestUtils.ts:export type TestData = { id: number; name: string; age: number; role: string; }; export const testColumns: Column<TestData>[] = [ { key: 'id', header: 'ID' }, { key: 'name', header: 'Name' }, { key: 'age', header: 'Age' }, { key: 'role', header: 'Role' }, ]; export const testRow: TestData = { id: 1, name: 'John Doe', age: 28, role: 'Developer' };packages/react-components/src/components/Table/styles/pin.scss (1)
11-33: Extract clip-path value to design tokenConsider moving the clip-path value to a design token for consistency and reusability.
- clip-path: inset(0 -15px 0 0); + clip-path: var(--clip-path-shadow);packages/react-components/src/components/Table/stories-helpers.tsx (1)
6-19: Move test data to separate JSON fileConsider moving the hardcoded arrays to a JSON file for better maintainability.
Also applies to: 21-38
packages/react-components/src/components/Table/stories-constants.tsx (1)
5-16: Consider adding column configuration optionsEnhance column definitions with additional properties like:
sortable: booleanresizable: booleanpinned: 'left' | 'right' | nullpackages/react-components/src/components/Table/TableBody.spec.tsx (1)
91-114: Enhance column width test assertionsThe current test only verifies cell widths. Add assertions for:
- Resizable columns
- Minimum/maximum width constraints
- Column width persistence
packages/react-components/src/components/Table/Table.stories.tsx (2)
58-133: Enhance story organization with compositionConsider using story composition to reduce code duplication:
+const Template: StoryFn = (args) => { + const getRowId = (row: Data) => row.id; + const data = generateData(4); + return <Table getRowId={getRowId} data={data} {...args} />; +}; -export const Default: StoryFn = () => { - const getRowId = (row: Data) => row.id; - const data = generateData(4); - return ( - <Table size="medium" data={data} columns={columns} getRowId={getRowId} /> - ); -}; +export const Default = Template.bind({}); +Default.args = { + size: "medium", + columns +};
135-165: Extract action bar component for reusabilityThe action bar implementation should be moved to a separate component for better reusability across stories.
packages/react-components/src/components/Table/Table.tsx (1)
155-163: Enhance error messagesAdd more context to error messages to help developers understand the issue:
- '`onSelectionChange` and `selectedRows` are required when selectable is true' + 'Table: `onSelectionChange` and `selectedRows` props are required when selectable={true}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
packages/react-components/src/components/Table/Table.mdx(1 hunks)packages/react-components/src/components/Table/Table.module.scss(1 hunks)packages/react-components/src/components/Table/Table.spec.tsx(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.module.scss(1 hunks)packages/react-components/src/components/Table/TableHeader.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.module.scss(1 hunks)packages/react-components/src/components/Table/TableRow.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.tsx(1 hunks)packages/react-components/src/components/Table/hooks.ts(1 hunks)packages/react-components/src/components/Table/index.ts(1 hunks)packages/react-components/src/components/Table/stories-constants.tsx(1 hunks)packages/react-components/src/components/Table/stories-helpers.tsx(1 hunks)packages/react-components/src/components/Table/styles/pin.scss(1 hunks)packages/react-components/src/components/Table/styles/size.scss(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/react-components/src/index.ts
- packages/react-components/src/components/Table/TableRow.module.scss
- packages/react-components/src/components/Table/styles/size.scss
- packages/react-components/src/components/Table/index.ts
- packages/react-components/src/components/Table/TableHeader.tsx
- packages/react-components/src/components/Table/Table.mdx
- packages/react-components/src/components/Table/TableHeader.module.scss
- packages/react-components/src/components/Table/hooks.ts
- packages/react-components/src/components/Table/Table.spec.tsx
- packages/react-components/src/components/Table/TableRow.tsx
🔇 Additional comments (13)
packages/react-components/src/components/Table/types.ts (4)
3-8: Add type constraint for sortable columns
When sortable is true, sortValue should be required to ensure proper sorting implementation.
56-65: Fix selectable prop type and add conditional requirements
The selectable prop should be boolean, and selectedRows should be required when selectable is true.
79-103: LGTM!
The data types are well-structured and provide good examples for both simple and complex use cases.
12-12:
Add support for right column pinning
According to the shadow requirements, right column pinning should be supported with --shadow-fixed-left.
-export type PinOptions = 'header' | 'leftColumn';
+export type PinOptions = 'header' | 'leftColumn' | 'rightColumn';Likely invalid or redundant comment.
packages/react-components/src/components/Table/TableRow.spec.tsx (1)
23-115: Add tests for missing table features
Based on the PR objectives, tests are needed for:
- Sorting functionality
- Column resizing
- Style specifications (heights, fonts, shadows)
packages/react-components/src/components/Table/TableBody.tsx (1)
25-38: 🛠️ Refactor suggestion
Optimize row rendering and add error handling
rowIdis calculated twice (lines 26 and 38)- Missing error boundary and empty state handling
- rowId={getRowId(row)}
+ rowId={rowId}Likely invalid or redundant comment.
packages/react-components/src/components/Table/stories-helpers.tsx (1)
48-48: Verify button size value
The xcompact size seems non-standard. Verify if this is a supported size in the Button component.
✅ Verification successful
xcompact is a valid Button size
The Button component explicitly supports xcompact as one of its size options, along with compact, medium, and large. This is defined in the component's stories and handled in the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for button size definitions
ast-grep --pattern 'size={$_}' packages/react-components/src/components/Button/
Length of output: 81
Script:
#!/bin/bash
# Search for Button component props interface or type definition
ast-grep --pattern 'interface ButtonProps {
$$$
}'
# Search for size prop usage in Button stories
rg -A 3 'size=' packages/react-components/src/components/Button/
# Search for size-related constants or types
rg -A 3 'size.*=' packages/react-components/src/components/Button/
Length of output: 6704
packages/react-components/src/components/Table/Table.module.scss (3)
32-36: Use design system typography token
Replace hardcoded values with paragraph-md token for cell text style.
24-26: Add shadow tokens for pinned elements
Add shadow tokens for pinned elements:
- Header:
--shadow-fixed-bottom - Left column:
--shadow-fixed-right
76-78: Add spacing between label and sorting icon
Add 8px spacing between label and sorting icon.
packages/react-components/src/components/Table/Table.stories.tsx (1)
10-14: Consider consolidating data generation logic
Move all data generation logic including column definitions to stories-helpers.ts for better maintainability.
Also applies to: 27-56
packages/react-components/src/components/Table/Table.tsx (2)
57-91:
Update column resizing behavior
Current implementation affects adjacent columns, which doesn't match requirements.
129-145:
Add null/undefined handling in sorting
The sorting implementation should handle null/undefined values gracefully.
| const columns: Column<Data>[] = [ | ||
| { key: 'id', header: 'ID', sortable: true }, | ||
| { key: 'name', header: 'Name', sortable: true }, | ||
| { key: 'age', header: 'Age', sortable: false }, | ||
| { key: 'role', header: 'Role', sortable: true }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tests for header styling requirements
Based on the PR objectives, add tests to verify:
- Header height (48px)
- Font size (paragraph-sm-bold)
- 8px spacing between label and sorting icon
| it('renders all columns', () => { | ||
| const { getAllByRole } = render( | ||
| <table> | ||
| <TableHeader | ||
| columns={columns} | ||
| sortConfig={sortConfig} | ||
| handleSort={mockHandleSort} | ||
| resizable={false} | ||
| handleMouseDown={mockHandleMouseDown} | ||
| columnRefs={columnRefs} | ||
| selectable={false} | ||
| hoveredColumnIndex={null} | ||
| setHoveredColumnIndex={setHoveredColumnIndex} | ||
| /> | ||
| </table> | ||
| ); | ||
|
|
||
| const headers = getAllByRole('columnheader'); | ||
| expect(headers.length).toBe(columns.length); | ||
| columns.forEach((col) => | ||
| expect(headers.some((header) => header.textContent === col.header)).toBe( | ||
| true | ||
| ) | ||
| ); | ||
| }); | ||
|
|
||
| it('calls handleSort when a sortable column is clicked', () => { | ||
| const { getByText } = render( | ||
| <table> | ||
| <TableHeader | ||
| columns={columns} | ||
| sortConfig={sortConfig} | ||
| handleSort={mockHandleSort} | ||
| resizable={false} | ||
| handleMouseDown={mockHandleMouseDown} | ||
| columnRefs={columnRefs} | ||
| selectable={false} | ||
| hoveredColumnIndex={null} | ||
| setHoveredColumnIndex={setHoveredColumnIndex} | ||
| /> | ||
| </table> | ||
| ); | ||
|
|
||
| const sortableColumn = getByText('ID'); | ||
| fireEvent.click(sortableColumn); | ||
|
|
||
| expect(mockHandleSort).toHaveBeenCalledWith('id'); | ||
| }); | ||
|
|
||
| it('does not call handleSort when a non-sortable column is clicked', () => { | ||
| const handleSort = vi.fn(); | ||
|
|
||
| const columns: Column<Data>[] = [ | ||
| { key: 'id', header: 'ID', sortable: true }, | ||
| { key: 'name', header: 'Name', sortable: false }, | ||
| ]; | ||
|
|
||
| const { getByText } = render( | ||
| <TableHeader | ||
| columns={columns} | ||
| sortConfig={{ key: null, direction: SortOrder.None }} | ||
| handleSort={handleSort} | ||
| resizable={false} | ||
| handleMouseDown={vi.fn()} | ||
| columnRefs={React.createRef()} | ||
| hoveredColumnIndex={null} | ||
| setHoveredColumnIndex={vi.fn()} | ||
| /> | ||
| ); | ||
|
|
||
| const nameHeader = getByText('Name'); | ||
| fireEvent.click(nameHeader); | ||
|
|
||
| expect(handleSort).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('handles resizable columns', () => { | ||
| const { getAllByRole } = render( | ||
| <table> | ||
| <TableHeader | ||
| columns={columns} | ||
| sortConfig={sortConfig} | ||
| handleSort={mockHandleSort} | ||
| resizable={true} | ||
| handleMouseDown={mockHandleMouseDown} | ||
| columnRefs={columnRefs} | ||
| selectable={false} | ||
| hoveredColumnIndex={null} | ||
| setHoveredColumnIndex={setHoveredColumnIndex} | ||
| /> | ||
| </table> | ||
| ); | ||
|
|
||
| const resizers = getAllByRole('button', { name: /resize/i }); | ||
| expect(resizers.length).toBe(columns.length); | ||
| fireEvent.mouseDown(resizers[0]); | ||
|
|
||
| expect(mockHandleMouseDown).toHaveBeenCalledWith(0, expect.any(Object)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing test cases for key requirements
Add test cases for:
- Column resizing affecting only the resized column (PR feedback mentions issues with both sides being affected)
- Shadow tokens for pinned headers
- Selectable state interactions
Example test structure:
it('resizes only the target column', () => {
// Setup with initial column widths
// Perform resize action
// Verify only target column width changed
});
it('applies correct shadow tokens when pinned', () => {
// Setup with pinned state
// Verify shadow token classes
});| describe('<TableHeader> component', () => { | ||
| const sortConfig = { key: null, direction: SortOrder.None }; | ||
| const mockHandleSort = vi.fn(); | ||
| const mockHandleMouseDown = vi.fn(); | ||
| const columnRefs = { current: Array(columns.length).fill(null) }; | ||
| const setHoveredColumnIndex = vi.fn(); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Add hover state test coverage
The implementation has hover-related logic (hoveredColumnIndex, hovered, and hover styles) but the test file only sets hoveredColumnIndex to null. Add test cases for:
- Column hover state changes
- Sort icon visibility on hover
- Style class application for hover states
🔗 Analysis chain
Add tests for sorting icon hover behavior
PR feedback mentions issues with sorting icon hover causing visual shifts. Add tests to verify hover behavior doesn't cause layout shifts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing hover-related tests in other spec files
rg -l "hover.*sort.*icon" --type ts
Length of output: 37
Script:
#!/bin/bash
# Let's check for any hover-related tests in the TableHeader spec
rg "hover" "packages/react-components/src/components/Table/TableHeader.spec.tsx"
# Also check the implementation file for hover handling
rg "hover" "packages/react-components/src/components/Table/TableHeader.tsx"
# Look for any sort icon related code
ast-grep --pattern 'const $_ = styled.$_`
$$$
&:hover {
$$$
}
$$$
`'
Length of output: 608
| it('should render a checkbox if selectable is true', () => { | ||
| const toggleRowSelection = vi.fn(); | ||
| const { getByRole } = render( | ||
| <table> | ||
| <tbody> | ||
| <TableRow | ||
| row={row} | ||
| columns={columns} | ||
| columnWidths={columnWidths} | ||
| selectable={true} | ||
| isSelected={(id) => id === row.id} | ||
| toggleRowSelection={toggleRowSelection} | ||
| rowId={row.id} | ||
| /> | ||
| </tbody> | ||
| </table> | ||
| ); | ||
|
|
||
| const checkbox = getByRole('checkbox'); | ||
| expect(checkbox).toBeChecked(); | ||
|
|
||
| fireEvent.click(checkbox); | ||
| expect(toggleRowSelection).toHaveBeenCalledWith(row.id); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add negative test case for row selection
The selection test should verify both selected and unselected states.
it('should handle unselected state correctly', () => {
const toggleRowSelection = vi.fn();
const { getByRole } = render(
<table>
<tbody>
<TableRow
row={row}
columns={columns}
columnWidths={columnWidths}
selectable={true}
isSelected={() => false}
toggleRowSelection={toggleRowSelection}
rowId={row.id}
/>
</tbody>
</table>
);
const checkbox = getByRole('checkbox');
expect(checkbox).not.toBeChecked();
});| it('should render aria-selected attribute correctly', () => { | ||
| const { getByRole } = render( | ||
| <table> | ||
| <tbody> | ||
| <TableRow | ||
| row={row} | ||
| columns={columns} | ||
| columnWidths={columnWidths} | ||
| selectable={true} | ||
| isSelected={(id) => id === row.id} | ||
| toggleRowSelection={() => {}} | ||
| rowId={row.id} | ||
| /> | ||
| </tbody> | ||
| </table> | ||
| ); | ||
|
|
||
| const cell = getByRole('cell', { name: '' }); | ||
| expect(cell).toHaveAttribute('aria-selected', 'true'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand accessibility testing
The accessibility test should verify more ARIA attributes and keyboard interactions.
it('should support keyboard selection', () => {
const toggleRowSelection = vi.fn();
const { getByRole } = render(
<table>
<tbody>
<TableRow
row={row}
columns={columns}
columnWidths={columnWidths}
selectable={true}
isSelected={() => false}
toggleRowSelection={toggleRowSelection}
rowId={row.id}
/>
</tbody>
</table>
);
const row = getByRole('row');
expect(row).toHaveAttribute('tabIndex', '0');
fireEvent.keyDown(row, { key: 'Space' });
expect(toggleRowSelection).toHaveBeenCalledWith(row.id);
});| it('should render a row with provided data and columns', () => { | ||
| const { getByText } = render( | ||
| <table> | ||
| <tbody> | ||
| <TableRow | ||
| row={row} | ||
| columns={columns} | ||
| columnWidths={columnWidths} | ||
| selectable={false} | ||
| isSelected={() => false} | ||
| toggleRowSelection={() => {}} | ||
| rowId={row.id} | ||
| /> | ||
| </tbody> | ||
| </table> | ||
| ); | ||
|
|
||
| expect(getByText('John Doe')).toBeInTheDocument(); | ||
| expect(getByText('28')).toBeInTheDocument(); | ||
| expect(getByText('Developer')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance basic rendering test coverage
The basic rendering test should verify all cell contents and their order.
- expect(getByText('John Doe')).toBeInTheDocument();
- expect(getByText('28')).toBeInTheDocument();
- expect(getByText('Developer')).toBeInTheDocument();
+ const cells = getAllByRole('cell');
+ expect(cells).toHaveLength(4);
+ expect(cells[0]).toHaveTextContent('1');
+ expect(cells[1]).toHaveTextContent('John Doe');
+ expect(cells[2]).toHaveTextContent('28');
+ expect(cells[3]).toHaveTextContent('Developer');Committable suggestion skipped: line range outside the PR's diff.
| @mixin generatePin($type) { | ||
| @if $type == 'header' { | ||
| thead { | ||
| position: sticky; | ||
| top: 0; | ||
| z-index: 5; | ||
| box-shadow: var(--shadow-fixed-bottom); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Z-index value needs standardization
Based on the search results, there's no consistent z-index scale in the design system. The codebase shows varying z-index values (1, 5, 120, 13001) without clear organization. The table header's z-index:5 appears arbitrary and should be standardized.
- Consider defining a z-index scale in design tokens
- Align table header z-index with other sticky elements
- Document the z-index hierarchy
🔗 Analysis chain
Verify z-index value in design system
The z-index value of 5 seems arbitrary. Ensure it aligns with the design system's z-index scale.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for z-index values in design system
rg -A 1 "z-index:|--z-index" packages/
Length of output: 5548
Script:
#!/bin/bash
# Search for z-index variables and stacking context definitions
rg "stacking-context-level|z-index:" packages/react-components/src/styles/
Length of output: 154
Script:
#!/bin/bash
# Search for SCSS variables and stacking context definitions
fd -e scss -x cat {} \; | rg "^\$.*stacking|^\$.*z-index"
Length of output: 60
| export const dataForPinningExample: DataForPinningExample[] = [ | ||
| { | ||
| userId: 1, | ||
| fullName: 'Alice Brown', | ||
| years: 29, | ||
| jobTitle: 'Software Engineer', | ||
| team: 'Development', | ||
| office: 'Seattle', | ||
| availability: 'Available', | ||
| workExperience: '4 years', | ||
| income: '$75,000', | ||
| controls: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| { | ||
| userId: 2, | ||
| fullName: 'Bob Green', | ||
| years: 35, | ||
| jobTitle: 'Product Manager', | ||
| team: 'Product', | ||
| office: 'Austin', | ||
| availability: 'Busy', | ||
| workExperience: '7 years', | ||
| income: '$110,000', | ||
| controls: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| { | ||
| userId: 3, | ||
| fullName: 'Charlie Davis', | ||
| years: 41, | ||
| jobTitle: 'UX Designer', | ||
| team: 'Design', | ||
| office: 'Denver', | ||
| availability: 'Available', | ||
| workExperience: '10 years', | ||
| income: '$95,000', | ||
| controls: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| { | ||
| userId: 4, | ||
| fullName: 'Dana White', | ||
| years: 26, | ||
| jobTitle: 'Data Analyst', | ||
| team: 'Analytics', | ||
| office: 'New York', | ||
| availability: 'On Leave', | ||
| workExperience: '2 years', | ||
| income: '$65,000', | ||
| controls: <Button size="xcompact">Edit</Button>, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Replace sensitive information in mock data
Consider replacing actual salary values with placeholder data to avoid exposing sensitive information in examples.
| describe('<TableBody> component', () => { | ||
| const getRowId = (row: Data) => row.id; | ||
|
|
||
| it('renders rows for each data entry', () => { | ||
| const { getAllByRole } = render( | ||
| <table> | ||
| <TableBody | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| columnWidths={[]} | ||
| selectable={false} | ||
| isSelected={() => false} | ||
| toggleRowSelection={() => {}} | ||
| /> | ||
| </table> | ||
| ); | ||
|
|
||
| const rows = getAllByRole('row'); | ||
| expect(rows.length).toBe(data.length); | ||
| }); | ||
|
|
||
| it('renders selectable rows with checkboxes when selectable is true', () => { | ||
| const { getAllByRole } = render( | ||
| <table> | ||
| <TableBody | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| columnWidths={[]} | ||
| selectable={true} | ||
| isSelected={(id) => id === 1} | ||
| toggleRowSelection={() => {}} | ||
| /> | ||
| </table> | ||
| ); | ||
|
|
||
| const checkboxes = getAllByRole('checkbox'); | ||
| expect(checkboxes.length).toBe(data.length); | ||
| expect(checkboxes[0]).toBeChecked(); | ||
| expect(checkboxes[1]).not.toBeChecked(); | ||
| }); | ||
|
|
||
| it('calls toggleRowSelection when a checkbox is clicked', () => { | ||
| const toggleRowSelection = vi.fn(); | ||
|
|
||
| const { getAllByRole } = render( | ||
| <table> | ||
| <TableBody | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| columnWidths={[]} | ||
| selectable={true} | ||
| isSelected={() => false} | ||
| toggleRowSelection={toggleRowSelection} | ||
| /> | ||
| </table> | ||
| ); | ||
|
|
||
| const checkboxes = getAllByRole('checkbox'); | ||
| fireEvent.click(checkboxes[1]); | ||
| expect(toggleRowSelection).toHaveBeenCalledWith(2); | ||
| }); | ||
|
|
||
| it('passes correct column widths to TableRow', () => { | ||
| const columnWidths = [100, 200, 150, 250]; | ||
| const { getAllByRole } = render( | ||
| <table> | ||
| <TableBody | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| columnWidths={columnWidths} | ||
| selectable={false} | ||
| isSelected={() => false} | ||
| toggleRowSelection={() => {}} | ||
| /> | ||
| </table> | ||
| ); | ||
|
|
||
| const cells = getAllByRole('cell'); | ||
| cells.forEach((cell, index) => { | ||
| const expectedWidth = columnWidths[index % columnWidths.length]; | ||
| if (expectedWidth) { | ||
| expect(cell).toHaveStyle(`width: ${expectedWidth}px`); | ||
| } | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for additional scenarios
Consider adding tests for:
- Empty data state
- Error handling
- Row hover states
- Striped rows styling
Example test case for empty state:
it('renders empty state when no data is provided', () => {
const { container } = render(
<table>
<TableBody
data={[]}
columns={columns}
getRowId={getRowId}
columnWidths={[]}
selectable={false}
isSelected={() => false}
toggleRowSelection={() => {}}
/>
</table>
);
expect(container.querySelector('tbody')?.children.length).toBe(0);
});| <table | ||
| className={cx( | ||
| styles[`${baseClass}`], | ||
| styles[`${baseClass}--${size}`], | ||
| styles[`${baseClass}--pinned_${pin}`], | ||
| { | ||
| [styles[`${baseClass}--stripped`]]: stripped, | ||
| } | ||
| )} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for accessibility
Add necessary ARIA attributes to improve accessibility:
<table
+ role="grid"
+ aria-label="Data table"
className={cx(
styles[`${baseClass}`],
styles[`${baseClass}--${size}`],
styles[`${baseClass}--pinned_${pin}`],
{
[styles[`${baseClass}--stripped`]]: stripped,
}
)}
>
|
Add please properties to turn on "Stripped” Rows or Columns like on example:
So the user of component can turn "Stripped " props for a specific rows or columns |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
packages/react-components/src/components/Table/Table.stories.tsx (1)
27-56: Consider using enum for column keysEnhance type safety by defining column keys as an enum instead of string literals.
+enum TableColumns { + ID = 'id', + NAME = 'name', + AGE = 'age', + ROLE = 'role', + ACTION = 'action' +} const columns: Column<Data>[] = [ { - key: 'id', + key: TableColumns.ID, header: 'ID', // ... }, // ... apply to other columns ];packages/react-components/src/components/Table/Table.tsx (2)
64-64: Document SCALING_FACTOR constantAdd a comment explaining why 1.05 was chosen as the scaling factor for column resizing.
+ // Multiplier to adjust mouse movement for smoother column resizing const SCALING_FACTOR = 1.05;
165-209: Consider extracting selection UI to a separate componentThe selection UI logic could be moved to a separate component to improve maintainability.
+ const SelectionBar = ({ selectedCount, toggleSelectAll, rowSelectionMessage, rowActions }) => ( + <div className={styles[`${selectedClass}`]}> + <Checkbox checked={selectedCount === data.length} onChange={toggleSelectAll} /> + {rowSelectionMessage || <Text size="md">{selectedCount} selected items</Text>} + {rowActions} + </div> + );packages/react-components/src/components/Table/types.ts (1)
88-107: Move example data types to separate filesThe
DataandDataForPinningExampletypes are sample data for examples. Consider moving them to example or test files to keep the component types file clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/react-components/src/components/Table/Table.module.scss(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)packages/react-components/src/components/Table/styles/pin.scss(1 hunks)packages/react-components/src/components/Table/styles/strips.scss(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-components/src/components/Table/styles/pin.scss
🔇 Additional comments (13)
packages/react-components/src/components/Table/Table.stories.tsx (4)
58-65: Implement required cell heights
According to design requirements, demonstrate all cell height variants:
- Small: 48px
- Medium: 56px
- Large: 64px
82-108: LGTM! Stripped rows implementation matches requirements
The implementation correctly demonstrates both row and column stripping variants.
110-132: Align header height with design specs
Set header height to 48px and use paragraph-sm-bold font as per requirements.
134-161: Add required shadow tokens for pinned elements
Implement shadow tokens as specified:
--shadow-fixed-bottomfor pinned header--shadow-fixed-rightfor left column--shadow-fixed-leftfor right column
packages/react-components/src/components/Table/styles/strips.scss (1)
17-19: Consider row-column hover interaction
The hover effect on striped columns might conflict with row hover states. Consider using a different hover color or implementing mutual exclusivity.
packages/react-components/src/components/Table/Table.module.scss (3)
25-27: Add shadow token for pinned header
Add --shadow-fixed-bottom token for pinned header.
33-37: Use design system typography tokens
Replace hardcoded values with design system tokens.
75-77: Add spacing between label and sorting icon
Add 8px spacing between label and sorting icon.
packages/react-components/src/components/Table/Table.tsx (3)
57-91: Modify column resizing behavior
Current implementation affects both columns during resize.
129-145: Handle undefined or null values during sorting
Add null checks in sorting logic.
179-186: Add ARIA attributes for accessibility
Add necessary ARIA attributes to improve table accessibility.
packages/react-components/src/components/Table/types.ts (2)
6-7: Make 'sortValue' required when 'sortable' is true
To ensure proper sorting functionality, make sortValue required when sortable is true.
Apply this diff:
export type Column<T> = {
key: keyof T;
header: string;
sortable?: boolean;
- sortValue?: (item: T) => string | number;
+ sortValue?: sortable extends true ? (item: T) => string | number : never;
};60-69: Update 'selectable' prop type and conditionally require selection props
Change selectable to a boolean, and make selectedRows and onSelectionChange required when selectable is true.
Apply this diff:
resizable?: boolean;
- selectable?: true;
+ selectable?: boolean;
- selectedRows?: Set<string | number>;
- onSelectionChange?: (selectedIds: Set<string | number>) => void;
+ selectedRows: selectable extends true ? Set<string | number> : never;
+ onSelectionChange: selectable extends true ? (selectedIds: Set<string | number>) => void : never;| export const Resizing: StoryFn = () => { | ||
| const getRowId = (row: Data) => row.id; | ||
| const data = generateData(4); | ||
|
|
||
| return ( | ||
| <Table | ||
| size="medium" | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| resizable | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Fix column resizing behavior
Current implementation affects both sides during resizing. Modify to only adjust the resized side.
| background-color: var(--surface-secondary-default); | ||
|
|
||
| &:hover { | ||
| background-color: var(--surface-primary-disabled) !important; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove !important flag
Avoid using !important as it creates specificity issues. Instead, increase selector specificity.
- background-color: var(--surface-primary-disabled) !important;
+ background-color: var(--surface-primary-disabled);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| background-color: var(--surface-primary-disabled) !important; | |
| background-color: var(--surface-primary-disabled); |
@vladko-uxds it's done. Could you check it? |
| #### Example implementation | ||
|
|
||
| ```jsx | ||
|
|
There was a problem hiding this comment.
Please add imports for components used in this example
|
Great job with this component. 💪 |
|
@arturfracala I see that when column is Stripped, the header still without it, the whole column (Cells + header) should have same background stripped.header.mp4 |
|
@arturfracala when column in pinned and we have a hover on a row - the whole row should have hover effect pinned cell too Uploading hover with pinned column.mp4… |
|
@arturfracala For resize columns please increase line width -> 2px |
arturfracala
force-pushed
the
feature/DPS-4377
branch
from
56710f8 to
0716e85
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/react-components/src/components/Table/types.ts (2)
10-10: Document size specifications in type definitionAdd JSDoc comment specifying the pixel heights: Small (48px), Medium (56px), Large (64px).
92-111: Move example types to stories fileData and DataForPinningExample appear to be example types. Consider moving them to the stories file to keep the core types file focused on reusable types.
packages/react-components/src/components/Table/TableBody.tsx (1)
14-43: Optimize row ID calculation.The
rowIdis calculated twice - once for the key and once as a prop. Consider using the cached value.return ( <tbody role="rowgroup"> {data.map((row) => { const rowId = getRowId(row); return ( <TableRow key={rowId} row={row} columns={columns} columnWidths={columnWidths} selectable={selectable} isSelected={isSelected} toggleRowSelection={toggleRowSelection} - rowId={getRowId(row)} + rowId={rowId} /> ); })} </tbody> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
packages/react-components/src/components/Table/Table.mdx(1 hunks)packages/react-components/src/components/Table/Table.module.scss(1 hunks)packages/react-components/src/components/Table/Table.spec.tsx(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.module.scss(1 hunks)packages/react-components/src/components/Table/TableHeader.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.module.scss(1 hunks)packages/react-components/src/components/Table/TableRow.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.tsx(1 hunks)packages/react-components/src/components/Table/hooks.ts(1 hunks)packages/react-components/src/components/Table/index.ts(1 hunks)packages/react-components/src/components/Table/stories-constants.tsx(1 hunks)packages/react-components/src/components/Table/stories-helpers.tsx(1 hunks)packages/react-components/src/components/Table/styles/pin.scss(1 hunks)packages/react-components/src/components/Table/styles/size.scss(1 hunks)packages/react-components/src/components/Table/styles/strips.scss(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/react-components/src/index.ts
- packages/react-components/src/components/Table/TableRow.module.scss
- packages/react-components/src/components/Table/stories-helpers.tsx
- packages/react-components/src/components/Table/styles/size.scss
- packages/react-components/src/components/Table/TableRow.spec.tsx
- packages/react-components/src/components/Table/index.ts
- packages/react-components/src/components/Table/styles/pin.scss
- packages/react-components/src/components/Table/Table.spec.tsx
- packages/react-components/src/components/Table/styles/strips.scss
- packages/react-components/src/components/Table/TableBody.spec.tsx
- packages/react-components/src/components/Table/TableHeader.tsx
- packages/react-components/src/components/Table/TableHeader.module.scss
- packages/react-components/src/components/Table/hooks.ts
- packages/react-components/src/components/Table/Table.mdx
- packages/react-components/src/components/Table/TableHeader.spec.tsx
- packages/react-components/src/components/Table/TableRow.tsx
- packages/react-components/src/components/Table/Table.module.scss
- packages/react-components/src/components/Table/stories-constants.tsx
🧰 Additional context used
📓 Learnings (1)
packages/react-components/src/components/Table/Table.tsx (1)
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.tsx:57-91
Timestamp: 2024-12-04T07:51:00.505Z
Learning: In the `Table` component, when implementing column resizing, ensure that resizing affects only the column being resized and does not adjust adjacent columns.
🔇 Additional comments (9)
packages/react-components/src/components/Table/types.ts (3)
87-90: LGTM!
The SortConfig interface is well-defined and properly typed.
3-8:
Enforce sortValue for sortable columns
When a column is marked as sortable, sortValue should be required to prevent runtime errors.
60-69:
Fix selectable prop type and dependencies
The current implementation has two issues:
- selectable should be boolean instead of literal
true - selectedRows and onSelectionChange should be required when selectable is true
packages/react-components/src/components/Table/TableBody.tsx (1)
4-12: LGTM! Well-structured props interface.
The interface provides good type safety and properly handles optional selection-related props.
packages/react-components/src/components/Table/Table.stories.tsx (2)
27-56: LGTM! Well-structured column definitions.
The column definitions properly implement sorting with appropriate sort value functions.
163-193: 🛠️ Refactor suggestion
Add keyboard navigation support.
Enhance accessibility by implementing keyboard navigation for row selection (e.g., Space to select, Arrow keys to navigate).
packages/react-components/src/components/Table/Table.tsx (3)
58-92:
Fix column resizing behavior.
According to previous feedback, column resizing should only affect the resized column, not its neighbors.
130-146:
Handle null/undefined values in sorting.
Add null checks to prevent runtime errors during sorting.
180-188: 🛠️ Refactor suggestion
Add ARIA attributes for accessibility.
Enhance screen reader support by adding appropriate ARIA attributes.
arturfracala
force-pushed
the
feature/DPS-4377
branch
from
0716e85 to
1739a96
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
packages/react-components/src/components/Table/stories-helpers.tsx (2)
6-38: Consider enhancing data diversity.The mock data could benefit from more international names and modern tech roles (e.g., "DevOps Engineer", "Cloud Architect", "ML Engineer").
44-44: Consider using UUID for id generation.Using incremental numbers for IDs could cause conflicts when combining datasets.
-id: index + 1, +id: crypto.randomUUID(),packages/react-components/src/components/Table/Table.stories.tsx (1)
171-179: Consider using semantic HTML for action bar dividers.Replace div-based dividers with
<hr>elements for better semantics.- const Divider = () => <div className={cx(styles[dividerClass])} />; + const Divider = () => <hr className={cx(styles[dividerClass])} />;packages/react-components/src/components/Table/Table.tsx (2)
38-44: Memoize table hook initializationWrap useTable hook initialization with useMemo to prevent unnecessary recalculations.
- const { isSelected, toggleRowSelection, toggleSelectAll, selectedCount } = - useTable({ + const tableHook = React.useMemo( + () => useTable({ data, getRowId, selectedRows: selectedRows, onSelectionChange: onSelectionChange, - }); + }), + [data, getRowId, selectedRows, onSelectionChange] + ); + const { isSelected, toggleRowSelection, toggleSelectAll, selectedCount } = tableHook;
149-154: Handle initialization errors gracefullyAdd error handling for column width initialization failures.
React.useEffect(() => { + try { const initialWidths = columnRefs.current.map( (ref) => ref?.offsetWidth || 0 ); setColumnWidths(initialWidths); + } catch (error) { + console.error('Failed to initialize column widths:', error); + setColumnWidths(Array(columns.length).fill(100)); // fallback width + } }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (22)
packages/react-components/src/components/Table/Table.mdx(1 hunks)packages/react-components/src/components/Table/Table.module.scss(1 hunks)packages/react-components/src/components/Table/Table.spec.tsx(1 hunks)packages/react-components/src/components/Table/Table.stories.tsx(1 hunks)packages/react-components/src/components/Table/Table.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableBody.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.module.scss(1 hunks)packages/react-components/src/components/Table/TableHeader.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableHeader.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.module.scss(1 hunks)packages/react-components/src/components/Table/TableRow.spec.tsx(1 hunks)packages/react-components/src/components/Table/TableRow.tsx(1 hunks)packages/react-components/src/components/Table/hooks.ts(1 hunks)packages/react-components/src/components/Table/index.ts(1 hunks)packages/react-components/src/components/Table/stories-constants.tsx(1 hunks)packages/react-components/src/components/Table/stories-helpers.tsx(1 hunks)packages/react-components/src/components/Table/styles/pin.scss(1 hunks)packages/react-components/src/components/Table/styles/size.scss(1 hunks)packages/react-components/src/components/Table/styles/strips.scss(1 hunks)packages/react-components/src/components/Table/types.ts(1 hunks)packages/react-components/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/react-components/src/index.ts
- packages/react-components/src/components/Table/TableRow.module.scss
- packages/react-components/src/components/Table/styles/size.scss
- packages/react-components/src/components/Table/styles/strips.scss
- packages/react-components/src/components/Table/styles/pin.scss
- packages/react-components/src/components/Table/TableRow.spec.tsx
- packages/react-components/src/components/Table/TableBody.spec.tsx
- packages/react-components/src/components/Table/index.ts
- packages/react-components/src/components/Table/Table.spec.tsx
- packages/react-components/src/components/Table/TableHeader.tsx
- packages/react-components/src/components/Table/TableRow.tsx
- packages/react-components/src/components/Table/TableHeader.spec.tsx
- packages/react-components/src/components/Table/Table.mdx
- packages/react-components/src/components/Table/hooks.ts
- packages/react-components/src/components/Table/TableHeader.module.scss
- packages/react-components/src/components/Table/stories-constants.tsx
- packages/react-components/src/components/Table/Table.module.scss
🧰 Additional context used
📓 Learnings (1)
packages/react-components/src/components/Table/Table.tsx (1)
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.tsx:57-91
Timestamp: 2024-12-04T07:51:00.505Z
Learning: In the `Table` component, when implementing column resizing, ensure that resizing affects only the column being resized and does not adjust adjacent columns.
🔇 Additional comments (8)
packages/react-components/src/components/Table/stories-helpers.tsx (2)
1-3: LGTM! Imports are clean and purposeful.
43-50: Verify Button size prop value.
Ensure that "xcompact" is a valid size option for the Button component.
✅ Verification successful
Button size "xcompact" is valid
The Button component explicitly supports 'xcompact' size as defined in packages/react-components/src/components/Button/types.ts:
export type ButtonSize = 'xcompact' | 'compact' | 'medium' | 'large';🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Button component size prop definition
ast-grep --pattern 'interface ButtonProps {
$$$
size?: $_;
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Let's try a broader search for Button component props and size-related definitions
rg -A 5 "type ButtonSize|interface ButtonProps|enum ButtonSize"
# Also search for actual usage of xcompact size in Button components
rg "size=.xcompact.|size=\"xcompact\""
# Search for any size-related constants or types for Button
rg "ButtonSize|BUTTON_SIZES"
Length of output: 2753
packages/react-components/src/components/Table/TableBody.tsx (2)
24-24: LGTM! Proper ARIA role implementation.
The role="rowgroup" attribute is correctly implemented for accessibility.
37-37:
Remove redundant rowId calculation.
The rowId is already calculated in line 26 and should be reused instead of calling getRowId again.
- rowId={getRowId(row)}
+ rowId={rowId}Likely invalid or redundant comment.
packages/react-components/src/components/Table/Table.tsx (4)
58-92:
Fix column resizing behavior per requirements
- Remove adjacent column width adjustment
- Add visual feedback with 2px resize handle
const handleMouseMove = (moveEvent: MouseEvent) => {
const delta = (moveEvent.clientX - startX) * SCALING_FACTOR;
setColumnWidths((prevWidths) => {
const newWidths = [...prevWidths];
const newWidth = Math.max(50, startWidth + delta);
- const adjustedNextWidth = Math.max(50, nextWidth - delta);
newWidths[index] = newWidth;
- if (index + 1 < newWidths.length) {
- newWidths[index + 1] = adjustedNextWidth;
- }
return newWidths;
});
};130-146:
Handle edge cases in sorting
Add null/undefined handling to prevent runtime errors.
return [...data].sort((a, b) => {
const sortValueA = column.sortValue
? column.sortValue(a)
: a[sortConfig.key as unknown as keyof T];
const sortValueB = column.sortValue
? column.sortValue(b)
: b[sortConfig.key as unknown as keyof T];
+
+ if (sortValueA == null && sortValueB == null) return 0;
+ if (sortValueA == null) return 1;
+ if (sortValueB == null) return -1;
if (sortValueA < sortValueB) {18-32: 🛠️ Refactor suggestion
Add accessibility attributes and size prop validation
Add ARIA attributes and validate size prop against the required dimensions:
- Small: 48px
- Medium: 56px
- Large: 64px
export const Table = <T,>({
data,
columns,
stripped,
- size = 'medium',
+ size = 'medium' as const,
pin,
selectable,
getRowId,
resizable,
selectedRows,
onSelectionChange,
rowSelectionMessage,
rowActions,
testId,
-}: ITableProps<T>) => {
+}: ITableProps<T> & { size?: 'small' | 'medium' | 'large' }) => {⛔ Skipped due to learnings
Learnt from: marcinsawicki
PR: livechat/design-system#1438
File: packages/react-components/src/components/Table/Table.mdx:11-11
Timestamp: 2024-12-04T07:31:14.364Z
Learning: When reviewing documentation files for the Table component, avoid suggesting adding specific sizing and styling specifications to the introduction unless explicitly requested.
189-199: 🛠️ Refactor suggestion
Implement header styling requirements
Add missing header specifications from PR objectives:
- Header height: 48px
- Font size: paragraph-sm-bold
- Icon spacing: 8px
| const getRandomElement = <T,>(arr: T[]): T => | ||
| arr[Math.floor(Math.random() * arr.length)]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety check for empty arrays.
The getRandomElement function should handle empty arrays to prevent undefined behavior.
-const getRandomElement = <T,>(arr: T[]): T =>
- arr[Math.floor(Math.random() * arr.length)];
+const getRandomElement = <T,>(arr: T[]): T => {
+ if (arr.length === 0) {
+ throw new Error('Cannot get random element from empty array');
+ }
+ return arr[Math.floor(Math.random() * arr.length)];
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getRandomElement = <T,>(arr: T[]): T => | |
| arr[Math.floor(Math.random() * arr.length)]; | |
| const getRandomElement = <T,>(arr: T[]): T => { | |
| if (arr.length === 0) { | |
| throw new Error('Cannot get random element from empty array'); | |
| } | |
| return arr[Math.floor(Math.random() * arr.length)]; | |
| }; |
| /** | ||
| * Enables row selection mode, adding checkboxes to each row and the header for selection. | ||
| */ | ||
| selectable?: true; |
There was a problem hiding this comment.
Fix selectable prop type and add conditional requirements.
The selectable prop should be boolean, and selectedRows/onSelectionChange should be required when selectable is true.
- selectable?: true;
+ selectable?: boolean;
- selectedRows?: Set<string | number>;
- onSelectionChange?: (selectedIds: Set<string | number>) => void;
+ selectedRows: selectable extends true ? Set<string | number> : never;
+ onSelectionChange: selectable extends true ? (selectedIds: Set<string | number>) => void : never;Also applies to: 64-64, 69-69
| export const MultiSelect: StoryFn = () => { | ||
| const getRowId = (row: Data) => row.id; | ||
| const data = generateData(4); | ||
| const [selectedRows, setSelectedRows] = React.useState<Set<string | number>>( | ||
| new Set() | ||
| ); | ||
|
|
||
| const Divider = () => <div className={cx(styles[dividerClass])} />; | ||
|
|
||
| const rowActions = ( | ||
| <div className={cx(styles[actionBarClass])}> | ||
| <Divider /> | ||
| <Button kind="link">Action</Button> <Button kind="link">Action</Button> | ||
| <Divider /> | ||
| <Button kind="link">Action</Button> <Button kind="link">Action</Button> | ||
| </div> | ||
| ); | ||
|
|
||
| return ( | ||
| <Table | ||
| size="medium" | ||
| data={data} | ||
| columns={columns} | ||
| getRowId={getRowId} | ||
| selectable | ||
| selectedRows={selectedRows} | ||
| onSelectionChange={setSelectedRows} | ||
| rowActions={rowActions} | ||
| /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility with keyboard navigation.
Add keyboard navigation support for row selection:
- Space to select/deselect
- Shift+Arrow for multiple selection
- Tab navigation between rows
Resolves: #{issue-number}
Description
The
Tablecomponent is a customizable solution for displaying tabular data. It supports features like sorting, column resizing, row selection, and striped row styling.Storybook
https://feature-DPS-4377--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
Release Notes
New Features
Tablecomponent with support for sorting, selection, and resizable columns.TableHeader,TableBody, andTableRowcomponents for structured table rendering.useTableto manage row selection logic.MultiSelectstory for selecting multiple rows in Storybook.Styling Enhancements
Documentation
Tablecomponent, detailing its features and usage examples.Tablecomponent.Type Definitions