feat(table): new TEDI-Ready component #122#624
Open
airikej wants to merge 36 commits into
Open
Conversation
…122-table-new-tedi-ready-component
…122-table-new-tedi-ready-component
21 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a TanStack-based generic Table with persistence, subcomponents, styles, tests, and Storybook stories; refactors Pagination for mobile (types, styling, breakpoint-aware rendering, and aria-live status); and includes supporting updates (Collapse external control, select styling, labels, and index re-export). ChangesTable Component Implementation
Pagination Component Enhancements
Supporting Component Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/tedi/components/content/table/table.stories.tsx (3)
1868-1874: ⚡ Quick winDrop the CSS
var()fallbacks in this style block.TEDI-ready styles intentionally omit fallback values, but this block reintroduces them for
borderRadiusandfontFamily. Use the tokens directly so the story stays aligned with the rest of the library. As per coding guidelines, "never use fallback values in CSS var()".Suggested cleanup
- borderRadius: 'var(--tedi-borders-radius-default, 4px)', - fontFamily: 'var(--family-mono, monospace)', + borderRadius: 'var(--tedi-borders-radius-default)', + fontFamily: 'var(--family-mono)',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.stories.tsx` around lines 1868 - 1874, The inline style object in the story uses CSS var() fallbacks for borderRadius and fontFamily (keys: borderRadius and fontFamily inside the style prop in table.stories.tsx); remove the fallback values so the tokens are used directly (e.g., change 'var(--tedi-borders-radius-default, 4px)' to 'var(--tedi-borders-radius-default)' and 'var(--family-mono, monospace)' to 'var(--family-mono)') to comply with TEDI guideline "never use fallback values in CSS var()".
185-198: 🏗️ Heavy liftMove the story-only presentation rules out of TSX.
This file starts introducing reusable
React.CSSPropertiesblocks here and then keeps layering more inline style literals through the story renderers. For TEDI stories, these styles should live in a CSS module so the examples follow the same styling path as the component itself. As per coding guidelines, "Use CSS Modules with SCSS for styling, avoiding inline styles".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.stories.tsx` around lines 185 - 198, The story file defines story-only inline style objects editLinkStyle and nameLinkStyle and uses more inline styles in the renderers; move these presentation rules into a CSS module (SCSS) and switch the story to use className instead of React.CSSProperties: create a table.stories.module.scss with classes (e.g. .editLink, .nameLink and any other story-specific styles), import that module into the story file, replace usages of editLinkStyle and nameLinkStyle in the story renderers with the corresponding module.className, remove the React.CSSProperties declarations, and convert any additional inline style literals in the render functions to module classes so the story follows the CSS Modules/SCSS styling guideline.
44-53: ⚡ Quick winAdd status metadata to the story meta export.
designis wired up, but TEDI stories are expected to publishstatusmetadata as well so Storybook shows component maturity consistently alongside the design link. As per coding guidelines, "meta parameters should include status/design info (including status.type array shape)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.stories.tsx` around lines 44 - 53, The story meta object named "meta" for the Table component is missing TEDI "status" metadata; update the meta export (the Meta<typeof Table> object) to include a status field alongside design under parameters, following the expected shape (e.g., a status property with a type array such as status: { type: ['stable'|'beta'|'deprecated' as appropriate] }); ensure you pick the correct maturity value for Table and add it to the meta.parameters so Storybook shows status and design together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tedi/components/content/table/table.spec.tsx`:
- Around line 418-488: The suite uses breakpoint-dependent markup but never
mocks the hook; add a single mock for useBreakpointProps at the top of the
describe('pagination') block (using jest.mock or jest.spyOn to stub
useBreakpointProps) and control its return value per-test (e.g., return
desktop:true for desktop-specific assertions and desktop:false for
mobile-specific assertions) so the rendered DOM is deterministic; update tests
that expect desktop controls (e.g., the page-size select assertions in the
'hides the page-size selector...' case and others like 'renders the pagination
bar...' and 'changes page size...') to rely on the mocked useBreakpointProps
return values.
In `@src/tedi/components/content/table/table.stories.tsx`:
- Around line 1556-1558: The footer currently computes the salary total using
info.table.getRowModel().rows which is post-pagination; change the aggregation
to use info.table.getFilteredRowModel().rows (or aggregate directly from the
people array) so the footer total matches the filtered/headcount scope shown in
the header; update the footer implementation (the footer function that reads
info.table.getRowModel().rows) to sum over info.table.getFilteredRowModel().rows
or people and keep formatting (toLocaleString('et-EE')) as before.
---
Nitpick comments:
In `@src/tedi/components/content/table/table.stories.tsx`:
- Around line 1868-1874: The inline style object in the story uses CSS var()
fallbacks for borderRadius and fontFamily (keys: borderRadius and fontFamily
inside the style prop in table.stories.tsx); remove the fallback values so the
tokens are used directly (e.g., change 'var(--tedi-borders-radius-default, 4px)'
to 'var(--tedi-borders-radius-default)' and 'var(--family-mono, monospace)' to
'var(--family-mono)') to comply with TEDI guideline "never use fallback values
in CSS var()".
- Around line 185-198: The story file defines story-only inline style objects
editLinkStyle and nameLinkStyle and uses more inline styles in the renderers;
move these presentation rules into a CSS module (SCSS) and switch the story to
use className instead of React.CSSProperties: create a table.stories.module.scss
with classes (e.g. .editLink, .nameLink and any other story-specific styles),
import that module into the story file, replace usages of editLinkStyle and
nameLinkStyle in the story renderers with the corresponding module.className,
remove the React.CSSProperties declarations, and convert any additional inline
style literals in the render functions to module classes so the story follows
the CSS Modules/SCSS styling guideline.
- Around line 44-53: The story meta object named "meta" for the Table component
is missing TEDI "status" metadata; update the meta export (the Meta<typeof
Table> object) to include a status field alongside design under parameters,
following the expected shape (e.g., a status property with a type array such as
status: { type: ['stable'|'beta'|'deprecated' as appropriate] }); ensure you
pick the correct maturity value for Table and add it to the meta.parameters so
Storybook shows status and design together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e23b7eaf-d0be-4bf3-a557-00598d31154f
📒 Files selected for processing (20)
src/community/components/table/table.stories.tsxsrc/tedi/components/buttons/collapse/collapse.tsxsrc/tedi/components/content/table/index.tssrc/tedi/components/content/table/table-columns-menu/table-columns-menu.tsxsrc/tedi/components/content/table/table-context.tsxsrc/tedi/components/content/table/table-header-button/table-header-button.module.scsssrc/tedi/components/content/table/table-header-button/table-header-button.tsxsrc/tedi/components/content/table/table.module.scsssrc/tedi/components/content/table/table.spec.tsxsrc/tedi/components/content/table/table.stories.tsxsrc/tedi/components/content/table/table.tsxsrc/tedi/components/content/table/table.types.tssrc/tedi/components/content/table/use-table-persistence.tssrc/tedi/components/form/select/select.module.scsssrc/tedi/components/navigation/pagination/pagination.module.scsssrc/tedi/components/navigation/pagination/pagination.spec.tsxsrc/tedi/components/navigation/pagination/pagination.tsxsrc/tedi/components/navigation/pagination/pagination.types.tssrc/tedi/index.tssrc/tedi/providers/label-provider/labels-map.ts
💤 Files with no reviewable changes (1)
- src/tedi/components/form/select/select.module.scss
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/tedi/components/content/table/table-header-button/table-header-button.spec.tsx (1)
14-17: ⚡ Quick winUse semantic button queries instead of
container.querySelector.Both tests can query the button with
screen.getByRole('button', { name: ... }), which keeps the suite aligned with RTL best practices and avoids DOM-structure coupling.Suggested test update
it('applies the selected modifier class when selected is true', () => { - const { container } = render(<TableHeaderButton icon="arrow_upward" selected aria-label="Sorted" />); - expect(container.querySelector('button')?.className).toMatch(/--selected/); + render(<TableHeaderButton icon="arrow_upward" selected aria-label="Sorted" />); + const button = screen.getByRole('button', { name: 'Sorted' }); + expect(button.className).toMatch(/--selected/); }); @@ it('merges custom className with the component class', () => { - const { container } = render(<TableHeaderButton icon="unfold_more" aria-label="Custom" className="custom-class" />); - const button = container.querySelector('button'); + render(<TableHeaderButton icon="unfold_more" aria-label="Custom" className="custom-class" />); + const button = screen.getByRole('button', { name: 'Custom' }); expect(button).toHaveClass('custom-class'); - expect(button?.className).toMatch(/tedi-table-header-button/); + expect(button.className).toMatch(/tedi-table-header-button/); });As per coding guidelines, "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)."Also applies to: 54-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table-header-button/table-header-button.spec.tsx` around lines 14 - 17, Replace non-semantic DOM queries with role-based queries: in the test "applies the selected modifier class when selected is true" (and the similar test around lines 54-59), stop using container.querySelector('button') and instead locate the element with screen.getByRole('button', { name: 'Sorted' }) after render(<TableHeaderButton ... />); then assert on that button's className or class list (e.g., expect(button.className).toMatch(/--selected/) or expect(button).toHaveClass(...)). Ensure you import/use screen from `@testing-library/react` and update both occurrences to use getByRole with the aria-label "Sorted".src/tedi/components/navigation/pagination/pagination.spec.tsx (2)
254-259: ⚡ Quick winPrefer semantic queries over id-prefix selectors in these assertions.
These checks currently use
container.querySelector(...), but the same intent is expressible via accessible-role queries and will be more resilient (e.g.,queryByRole('combobox', { name: /Page size/i }),getByRole('combobox', { name: /Pagination/i })).As per coding guidelines, "Use semantic queries in tests (
getByRole,getByLabelText) instead of non-semantic queries (getByTestId)."Also applies to: 307-309, 316-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/navigation/pagination/pagination.spec.tsx` around lines 254 - 259, Replace non-semantic container.querySelector('[id^="tedi-pagination-page-size-"]') checks in the Pagination tests with accessible role queries: after rendering Pagination (render(<Pagination ... />)), use queryByRole('combobox', { name: /Page size/i }) to assert the page-size select is not present and use getByRole('combobox', { name: /Pagination/i }) or the appropriate accessible name for the page-jump select where presence is expected; update the other instances referenced (around lines 307-309 and 316-317) similarly to use queryByRole/getByRole instead of container.querySelector to follow semantic query guidelines.
20-22: ⚡ Quick winAvoid
anyin the breakpoint-props mock.Line 21 uses
props: any. This can be typed generically without losing flexibility.Suggested change
- // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - getCurrentBreakpointProps: (props: any) => ({ ...props }), + getCurrentBreakpointProps: <T extends Record<string, unknown>>(props: T): T => ({ ...props }),As per coding guidelines, "Use proper TypeScript generics instead of
@ts-ignoreoranytypes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/navigation/pagination/pagination.spec.tsx` around lines 20 - 22, The mock uses an explicit any in getCurrentBreakpointProps which violates typing rules; change the mock to a generic signature so it preserves input types (e.g., make getCurrentBreakpointProps generic like <T>(props: T) => T or equivalent) so callers retain proper types instead of any; update the mock declaration for getCurrentBreakpointProps in the pagination spec to use that generic type (referencing getCurrentBreakpointProps) and adjust any test usages if needed.src/tedi/components/buttons/collapse/collapse.spec.tsx (1)
154-165: ⚡ Quick winConsider verifying DOM state in addition to callback values.
The test correctly verifies that
onToggleis called with the appropriate state values (truefor Enter,falsefor Space). To make the test more robust, consider also asserting the actual DOM state changes—for example, checking thearia-expandedattribute on the button or verifying the content element's visibility.🧪 Suggested enhancement
it('toggles open state when Enter or Space is pressed on the trigger', () => { const onToggle = jest.fn(); const { getByRole } = getComponent({ id: 'collapse-keyboard', onToggle }); const button = getByRole('button', { name: /näita rohkem/i }); fireEvent.keyDown(button, { key: 'Enter' }); expect(onToggle).toHaveBeenCalledWith(true); + expect(button).toHaveAttribute('aria-expanded', 'true'); fireEvent.keyDown(button, { key: ' ' }); expect(onToggle).toHaveBeenCalledWith(false); + expect(button).toHaveAttribute('aria-expanded', 'false'); expect(onToggle).toHaveBeenCalledTimes(2); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/buttons/collapse/collapse.spec.tsx` around lines 154 - 165, The test currently asserts the onToggle mock calls but not the resulting DOM changes; update the spec in collapse.spec.tsx (the test using getComponent and the button variable) to also assert the button's aria-expanded attribute and the collapse content visibility after each key press—e.g. after firing Enter assert button.getAttribute('aria-expanded') === 'true' and that the content element is visible, then after firing Space assert aria-expanded === 'false' and the content is hidden—so both callback behavior (onToggle) and actual DOM state are validated.src/tedi/components/content/table/table.stories.tsx (2)
44-53: ⚡ Quick winStorybook meta is missing
parameters.status.type.Per the contributing best-practices, story
metamust include aparameters.status.type. Onlyparameters.designis provided here.♻️ Suggested addition
const meta: Meta<typeof Table> = { component: Table, title: 'TEDI-Ready/Content/Table', parameters: { + status: { + type: /* the appropriate status, e.g. 'beta' | 'stable' */, + }, design: { type: 'figma', url: 'https://www.figma.com/design/jWiRIXhHRxwVdMSimKX2FF/TEDI-READY-2.45.70?node-id=4514-63761&m=dev', }, }, };As per coding guidelines: "Storybook: meta must include parameters.status.type; follow the provided status/design parameter pattern".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.stories.tsx` around lines 44 - 53, The storybook meta object (meta: Meta<typeof Table> for component Table) is missing parameters.status.type; update the meta.parameters to include a status entry (e.g., parameters.status = { type: '<appropriate-status-value>' }) alongside the existing design entry so the meta includes both parameters.design and parameters.status.type as required by the contributing guidelines.
1420-1428: 💤 Low valueAwkward
<></>children on icon-only Buttons.Several icon-only
Buttonusages pass an empty fragment as children plus anonClick={() => undefined}no-op. IfButtonsupports an icon-only mode (no required visible label / no children), prefer that pattern; otherwise consider rendering an accessible visually-hidden label child instead of an empty fragment. Repeating() => undefinedfor click handlers also obscures the demo's intent — either omit the handler if the prop is optional, or wire it to a real action (e.g., aconsole.infoplaceholder).Also applies to: 1442-1446, 1510-1518
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.stories.tsx` around lines 1420 - 1428, The Button usages rendering icon-only controls (Button with icon="edit"/"delete" in the row actions span) should not pass an empty fragment as children or a no-op onClick; either use Button's icon-only mode (remove the empty fragment children entirely) or provide an accessible visually-hidden label child, and replace onClick={() => undefined} with either nothing (omit the prop if optional) or a real placeholder like () => console.info('edit')/() => console.info('delete'); apply the same changes to the other occurrences referenced (the Button instances at the other noted blocks).src/tedi/components/content/table/table.spec.tsx (1)
217-246: 💤 Low value
pageCount={data.length}is semantically incorrect.
pageCountis the total number of pages, not rows. Here it happens to evaluate to2, which equalsceil(data.length / pageSize)only becausedata.length === 2andpageSize === 1. A future change todatawill silently desync the test. Consider expressing it aspageCount={Math.ceil(data.length / pagination.pageSize)}or a literalpageCount={2}with a comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.spec.tsx` around lines 217 - 246, The test passes an incorrect semantic value for the Table prop pageCount (it currently uses data.length); update the ServerSideWrapper so pageCount reflects total pages rather than rows — e.g. compute pageCount as Math.ceil(data.length / pagination.pageSize) using the pagination state from ServerSideWrapper (or replace with a fixed literal like 2 plus a comment explaining why) so the Table prop pageCount is correct when calling <Table ... pageCount={...}>.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tedi/components/content/table/table.spec.tsx`:
- Around line 307-324: The test replaces window.localStorage with a throwing
getter but only restores it when an original descriptor exists; to avoid leaking
the mocked getter into other tests, capture the original via
Object.getOwnPropertyDescriptor(window, 'localStorage') and in the finally block
restore using Object.defineProperty(window, 'localStorage', original) if
original is present, otherwise remove the own property by doing delete (window
as { localStorage?: unknown }).localStorage so prototype-backed localStorage is
re-exposed; adjust the finally in the 'falls back gracefully when
window.localStorage access throws' test around render(<Table... persist={{ key:
'persist-block' }} />) accordingly.
In `@src/tedi/components/content/table/table.stories.tsx`:
- Around line 1870-1879: In the inline style object for the <pre> JSX element in
table.stories.tsx, remove the CSS var() fallback values so they match project
convention; specifically replace occurrences of
var(--tedi-borders-radius-default, 4px) with var(--tedi-borders-radius-default)
and var(--family-mono, monospace) with var(--family-mono) (update any similar
var(..., fallback) usages in that style object).
In `@src/tedi/components/navigation/pagination/pagination.module.scss`:
- Around line 150-157: Replace the hardcoded "transparent" values in the
breakpoints.media-breakpoint-down(md) block and its nested &:hover/:active rules
with the core design token: import the design tokens from
'@tedi-design-system/core' (e.g. as tokens) and use the transparent color token
(e.g. tokens.color.transparent or the project’s transparent token name) for the
background declarations; update both occurrences inside the selector containing
&:hover:not(:disabled, .tedi-pagination__button--selected) and
&:active:not(:disabled, .tedi-pagination__button--selected) so the component
uses the tokenized value instead of the literal "transparent".
---
Nitpick comments:
In `@src/tedi/components/buttons/collapse/collapse.spec.tsx`:
- Around line 154-165: The test currently asserts the onToggle mock calls but
not the resulting DOM changes; update the spec in collapse.spec.tsx (the test
using getComponent and the button variable) to also assert the button's
aria-expanded attribute and the collapse content visibility after each key
press—e.g. after firing Enter assert button.getAttribute('aria-expanded') ===
'true' and that the content element is visible, then after firing Space assert
aria-expanded === 'false' and the content is hidden—so both callback behavior
(onToggle) and actual DOM state are validated.
In
`@src/tedi/components/content/table/table-header-button/table-header-button.spec.tsx`:
- Around line 14-17: Replace non-semantic DOM queries with role-based queries:
in the test "applies the selected modifier class when selected is true" (and the
similar test around lines 54-59), stop using container.querySelector('button')
and instead locate the element with screen.getByRole('button', { name: 'Sorted'
}) after render(<TableHeaderButton ... />); then assert on that button's
className or class list (e.g., expect(button.className).toMatch(/--selected/) or
expect(button).toHaveClass(...)). Ensure you import/use screen from
`@testing-library/react` and update both occurrences to use getByRole with the
aria-label "Sorted".
In `@src/tedi/components/content/table/table.spec.tsx`:
- Around line 217-246: The test passes an incorrect semantic value for the Table
prop pageCount (it currently uses data.length); update the ServerSideWrapper so
pageCount reflects total pages rather than rows — e.g. compute pageCount as
Math.ceil(data.length / pagination.pageSize) using the pagination state from
ServerSideWrapper (or replace with a fixed literal like 2 plus a comment
explaining why) so the Table prop pageCount is correct when calling <Table ...
pageCount={...}>.
In `@src/tedi/components/content/table/table.stories.tsx`:
- Around line 44-53: The storybook meta object (meta: Meta<typeof Table> for
component Table) is missing parameters.status.type; update the meta.parameters
to include a status entry (e.g., parameters.status = { type:
'<appropriate-status-value>' }) alongside the existing design entry so the meta
includes both parameters.design and parameters.status.type as required by the
contributing guidelines.
- Around line 1420-1428: The Button usages rendering icon-only controls (Button
with icon="edit"/"delete" in the row actions span) should not pass an empty
fragment as children or a no-op onClick; either use Button's icon-only mode
(remove the empty fragment children entirely) or provide an accessible
visually-hidden label child, and replace onClick={() => undefined} with either
nothing (omit the prop if optional) or a real placeholder like () =>
console.info('edit')/() => console.info('delete'); apply the same changes to the
other occurrences referenced (the Button instances at the other noted blocks).
In `@src/tedi/components/navigation/pagination/pagination.spec.tsx`:
- Around line 254-259: Replace non-semantic
container.querySelector('[id^="tedi-pagination-page-size-"]') checks in the
Pagination tests with accessible role queries: after rendering Pagination
(render(<Pagination ... />)), use queryByRole('combobox', { name: /Page size/i
}) to assert the page-size select is not present and use getByRole('combobox', {
name: /Pagination/i }) or the appropriate accessible name for the page-jump
select where presence is expected; update the other instances referenced (around
lines 307-309 and 316-317) similarly to use queryByRole/getByRole instead of
container.querySelector to follow semantic query guidelines.
- Around line 20-22: The mock uses an explicit any in getCurrentBreakpointProps
which violates typing rules; change the mock to a generic signature so it
preserves input types (e.g., make getCurrentBreakpointProps generic like
<T>(props: T) => T or equivalent) so callers retain proper types instead of any;
update the mock declaration for getCurrentBreakpointProps in the pagination spec
to use that generic type (referencing getCurrentBreakpointProps) and adjust any
test usages if needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f4f0af9-1f89-4b64-ba6a-ab119bd3e24b
📒 Files selected for processing (13)
src/tedi/components/buttons/collapse/collapse.spec.tsxsrc/tedi/components/content/table/index.tssrc/tedi/components/content/table/table-columns-menu/table-columns-menu.tsxsrc/tedi/components/content/table/table-header-button/table-header-button.module.scsssrc/tedi/components/content/table/table-header-button/table-header-button.spec.tsxsrc/tedi/components/content/table/table.module.scsssrc/tedi/components/content/table/table.spec.tsxsrc/tedi/components/content/table/table.stories.tsxsrc/tedi/components/content/table/table.tsxsrc/tedi/components/content/table/table.types.tssrc/tedi/components/navigation/pagination/pagination.module.scsssrc/tedi/components/navigation/pagination/pagination.spec.tsxsrc/tedi/components/navigation/pagination/pagination.tsx
✅ Files skipped from review due to trivial changes (2)
- src/tedi/components/content/table/table-header-button/table-header-button.module.scss
- src/tedi/components/content/table/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tedi/components/content/table/table.types.ts
- src/tedi/components/content/table/table-columns-menu/table-columns-menu.tsx
- src/tedi/components/content/table/table.tsx
- src/tedi/components/navigation/pagination/pagination.tsx
- src/tedi/components/content/table/table.module.scss
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tedi/components/content/table/table.tsx`:
- Around line 657-677: The top-row standalone headers currently get rowSpan=1
causing blanks under them; update the rowSpan calculation so non-placeholder
standalone headers (where isGroup is false and hasParentGroup is false) span the
remaining header depth: compute rowSpanCount = header.isPlaceholder ?
headerGroups.length - rowIndex : (!isGroup && !hasParentGroup ?
headerGroups.length - rowIndex : 1). Adjust the logic around
header.isPlaceholder / isGroup / hasParentGroup in the block that returns the
<th> (using symbols header, headerGroups, rowIndex, isGroup, hasParentGroup) so
the top-level leaf header spans the full header depth instead of leaving an
empty slot.
- Around line 466-476: The header Checkbox currently uses
table.getIsAllRowsSelected(), table.getIsSomeRowsSelected(), and
table.toggleAllRowsSelected(), which select across the entire dataset; change
these to the page-scoped APIs so pagination only affects the visible page: use
table.getIsAllPageRowsSelected() for checked, table.getIsSomePageRowsSelected()
for indeterminate calculation, and call table.toggleAllPageRowsSelected(checked)
in the onChange handler; keep the same props (id/name/resolvedId/getLabelRef)
but swap the global selection calls to the page-scoped counterparts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1ad6f7b-ea73-4c12-9448-1575616b8cc7
📒 Files selected for processing (11)
src/tedi/components/content/table/index.tssrc/tedi/components/content/table/table-context.tsxsrc/tedi/components/content/table/table-toolbar/table-toolbar.tsxsrc/tedi/components/content/table/table.module.scsssrc/tedi/components/content/table/table.spec.tsxsrc/tedi/components/content/table/table.stories.tsxsrc/tedi/components/content/table/table.tsxsrc/tedi/components/content/table/use-table-persistence.tssrc/tedi/components/navigation/pagination/pagination.spec.tsxsrc/tedi/components/navigation/pagination/pagination.tsxsrc/tedi/providers/label-provider/labels-map.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tedi/components/content/table/table-context.tsx
- src/tedi/components/content/table/index.ts
- src/tedi/providers/label-provider/labels-map.ts
- src/tedi/components/navigation/pagination/pagination.spec.tsx
- src/tedi/components/content/table/use-table-persistence.ts
- src/tedi/components/content/table/table.module.scss
- src/tedi/components/navigation/pagination/pagination.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tedi/components/content/table/table.stories.tsx (1)
793-927: ⚡ Quick winConsider reusing the shared
useEditableRows+EditableRowsContextpattern.This story re-implements the inline editing logic that's already available via the shared utilities (lines 240-267, 218-232). The shared pattern keeps the columns array stable (avoiding TanStack Table reconciliation), whereas this implementation requires
useMemowith[draft, editingId]dependencies.If the duplication is intentional to demonstrate alternative approaches, consider adding a comment explaining the trade-off. Otherwise, refactoring to reuse the shared pattern would reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.stories.tsx` around lines 793 - 927, The story EditableValues reimplements inline-editing state (beginEdit, cancelEdit, commitEdit, draft, editingId) and builds columns in a useMemo dependent on [draft, editingId], duplicating the shared useEditableRows + EditableRowsContext pattern; refactor this story to consume the existing useEditableRows hook and/or EditableRowsContext (replace local state and handlers with the shared hook's API) and move editable cell rendering to the stable columns built without draft/editingId dependencies so the columns array stays stable for TanStack Table reconciliation; if you intentionally want to show an alternative, add a short comment in EditableValues explaining the trade-off and why it diverges from useEditableRows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tedi/components/content/table/table.stories.tsx`:
- Around line 793-927: The story EditableValues reimplements inline-editing
state (beginEdit, cancelEdit, commitEdit, draft, editingId) and builds columns
in a useMemo dependent on [draft, editingId], duplicating the shared
useEditableRows + EditableRowsContext pattern; refactor this story to consume
the existing useEditableRows hook and/or EditableRowsContext (replace local
state and handlers with the shared hook's API) and move editable cell rendering
to the stable columns built without draft/editingId dependencies so the columns
array stays stable for TanStack Table reconciliation; if you intentionally want
to show an alternative, add a short comment in EditableValues explaining the
trade-off and why it diverges from useEditableRows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95c02b92-5332-46dd-a2e8-4d6218185a6c
📒 Files selected for processing (4)
src/tedi/components/content/table/table.module.scsssrc/tedi/components/content/table/table.spec.tsxsrc/tedi/components/content/table/table.stories.tsxsrc/tedi/components/content/table/table.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tedi/components/content/table/table.module.scss
- src/tedi/components/content/table/table.spec.tsx
- src/tedi/components/content/table/table.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tedi/components/content/table/table.stories.tsx (1)
303-315: ⚡ Quick winConsider using
<button>instead of<a href="#">for non-navigation actions.When an action doesn't navigate to a new URL, use a
<button>element styled as a link rather than an<a>with a#href. This improves semantics and accessibility — screen readers announce it correctly as a button action rather than a navigation link.♻️ Suggested pattern
- <a - href="#" - onClick={(event) => { - event.preventDefault(); - editor.beginEdit(row); - }} - style={editLinkStyle} - > + <button + type="button" + onClick={() => editor.beginEdit(row)} + style={editLinkStyle} + > <Icon name="edit" color="brand" size={18} /> Muuda - </a> + </button>Apply the same change at lines 478 and 1454 where the same pattern appears.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tedi/components/content/table/table.stories.tsx` around lines 303 - 315, Replace the non-navigation anchor used for the edit action with a semantic button: instead of <a href="#"> using editRowActionsStyle/editLinkStyle and calling editor.beginEdit(row) in the onClick, use a <button type="button"> styled with editLinkStyle (or a className) and keep the same onClick handler (call editor.beginEdit(row) and preventDefault is no longer needed); ensure accessibility attributes remain (e.g., aria-label if present) and preserve the Icon and text; apply the identical replacement for the other occurrences that use the same pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/tedi/components/content/table/table.stories.tsx`:
- Around line 303-315: Replace the non-navigation anchor used for the edit
action with a semantic button: instead of <a href="#"> using
editRowActionsStyle/editLinkStyle and calling editor.beginEdit(row) in the
onClick, use a <button type="button"> styled with editLinkStyle (or a className)
and keep the same onClick handler (call editor.beginEdit(row) and preventDefault
is no longer needed); ensure accessibility attributes remain (e.g., aria-label
if present) and preserve the Icon and text; apply the identical replacement for
the other occurrences that use the same pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3bdfdf6-3969-4066-a200-77c1b8ded643
📒 Files selected for processing (11)
skills/tedi-react/references/components.mdsrc/tedi/components/content/table/table-columns-menu/table-columns-menu.tsxsrc/tedi/components/content/table/table.module.scsssrc/tedi/components/content/table/table.spec.tsxsrc/tedi/components/content/table/table.stories.tsxsrc/tedi/components/content/table/table.tsxsrc/tedi/components/navigation/pagination/pagination.module.scsssrc/tedi/components/navigation/pagination/pagination.spec.tsxsrc/tedi/components/navigation/pagination/pagination.tsxsrc/tedi/index.tssrc/tedi/providers/label-provider/labels-map.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tedi/components/content/table/table-columns-menu/table-columns-menu.tsx
- src/tedi/components/navigation/pagination/pagination.module.scss
- src/tedi/components/navigation/pagination/pagination.spec.tsx
- src/tedi/providers/label-provider/labels-map.ts
- src/tedi/components/content/table/table.module.scss
- src/tedi/components/content/table/table.tsx
- src/tedi/components/content/table/table.spec.tsx
Contributor
Author
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Enhancements
Tests