Skip to content

feat(table): 🎸 add isResponsive prop to control mobile layout behavior#881

Open
punkbit wants to merge 19 commits intomainfrom
feat/table-disable-mobile-list-view-reviewed
Open

feat(table): 🎸 add isResponsive prop to control mobile layout behavior#881
punkbit wants to merge 19 commits intomainfrom
feat/table-disable-mobile-list-view-reviewed

Conversation

@punkbit
Copy link
Collaborator

@punkbit punkbit commented Mar 6, 2026

Why?

Adds a new isResponsive prop to the Table component. When set to true, the table maintains its standard layout with horizontal scroll on narrow screens instead of automatically converting to a mobile list view. The default behavior (isResponsive={false}) remains unchanged.

The implementation uses a data-responsive-mode data attribute to conditionally apply mobile list styles.

🍰 Original contribution by @maxknv maxknv (#877), retained credit/commit history.

How?

  • Add disableMobileListView?: boolean prop (default: false)
  • Set data-responsive-mode attribute on TableOuterContainer
  • Wrap all mobile @media queries with [data-responsive-mode="list"] & selector
  • When disableMobileListView=true, mobile styles don't apply

Preview?

demo-table-is-responsive-control.mov

maxknv and others added 5 commits March 5, 2026 22:25
…nversion

Add new prop to Table component that disables the automatic conversion to
mobile list view on narrow screens, keeping the table layout with horizontal
scroll instead.

Changes:
- Add disableMobileListView?: boolean prop (default: false)
- Set data-responsive-mode attribute on TableOuterContainer
- Wrap all mobile @media queries with [data-responsive-mode="list"] & selector
- When disableMobileListView=true, mobile styles don't apply

Benefits:
- Simple implementation (3 main changes)
- No prop threading through nested components
- Clean CSS selector approach
- Backward compatible (default behavior unchanged)

Use case: When you want tables to scroll horizontally on mobile instead
of converting to a list view format.
@changeset-bot
Copy link

changeset-bot bot commented Mar 6, 2026

🦋 Changeset detected

Latest commit: 2420da6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 34s —— View job


Code Review

Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Nice work on migrating from hardcoded 768px to the theme breakpoint token and using a data-* attribute for the styling hook — that follows the repo's conventions well.

There are a few things worth addressing before merging:

🚨 Must Fix

  • TableWrapper missing overflow-x: auto: When isResponsive={true}, all the mobile list-view styles are correctly suppressed, but no horizontal scroll is enabled. TableWrapper has overflow: visible, so on narrow screens the table will silently overflow rather than scroll. The PR description promises "horizontal scroll instead" — this is the missing half of that. You'll need to either add overflow-x: auto to TableWrapper conditionally (perhaps via a prop), or add it unconditionally since scroll only activates when content overflows. Fix this →

⚠️ Missing Changeset

  • The changeset bot already flagged this: a new public prop (isResponsive) was added to TableProps, so a minor changeset is needed. Per .llm/CONVENTIONS.md, run yarn changeset:add and include a usage example. Fix this →

💡 Suggestions (non-blocking)

  • Prop semantics are a bit inverted: isResponsive={false} (the default) is actually the responsive behavior (it adapts to mobile by showing a list view). isResponsive={true} opts into scroll-only. Naming like responsiveMode?: 'list' | 'scroll' or mobileLayout?: 'list' | 'scroll' would make the intent unambiguous. The current name could confuse future users who expect isResponsive={true} to enable responsive behavior.

  • Inconsistent quote style in CSS selectors: Some styled components use [data-responsive-mode='list'] (single quotes) while others use [data-responsive-mode="list"] (double quotes). Both are valid CSS but picking one is cleaner.

  • No tests or stories for the new prop: The test file has no coverage for isResponsive, and the story file has no new story for the scroll-mode behavior. Per conventions, new behavioral states should be tested and have Storybook stories.

  • Pre-existing: ActionsList still uses a hardcoded border-left: 1px solid (line 482) instead of theme.click.table.cell.stroke — not introduced by this PR but worth a follow-up.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 10s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Nice clean approach using a data-* attribute as a styling hook — that's the right pattern for this repo. The theme breakpoint migration is good too. A few things still need attention before merging:

🚨 Must Fix

  • TableWrapper missing overflow-x: auto (Table.tsx:501): TableWrapper still has overflow: visible. When isResponsive={true} (scroll mode), the table will silently overflow its container on narrow screens rather than scrolling. The PR description promises "horizontal scroll instead" — this is the missing half of that. Adding overflow-x: auto to TableWrapper unconditionally is safe since scroll only activates when content overflows. Fix this →

⚠️ Missing Changeset

  • A new public prop (isResponsive) was added to TableProps — a minor changeset is required. Per .llm/CONVENTIONS.md, run yarn changeset:add and include a usage example. Fix this →

💡 Suggestions (non-blocking)

  • Inverted prop semantics (Table.tsx:968): isResponsive={false} (the default) produces data-responsive-mode="list" — which IS the responsive behavior. isResponsive={true} disables it. This is counterintuitive: users expect isResponsive={true} to mean "this is responsive." The original disableMobileListView or a union like mobileLayout?: 'list' | 'scroll' would make intent unambiguous.

  • Inconsistent CSS attribute quote style: Some styled components use single quotes ([data-responsive-mode='list'], e.g. lines 354, 398, 413) while others use double quotes ([data-responsive-mode="list"], e.g. lines 386, 426, 449). Both are valid CSS — pick one and be consistent.

  • No tests or stories: Per conventions (Component.test.tsx, Storybook stories for all states), the new isResponsive behavior should have a test asserting the data-responsive-mode attribute value, and a Storybook story demonstrating the scroll-mode layout.

@punkbit punkbit changed the title feat(table): 🎸 table disable mobile list view (reviewed) feat(table): 🎸 add isResponsive prop to control mobile layout behavior Mar 6, 2026
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 7s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Good progress — the changeset is now present and well-formed. The hardcoded 768px → theme breakpoint migration is solid. One must-fix from the previous two reviews is still open:

🚨 Must Fix

  • TableWrapper still has overflow: visible (Table.tsx:504): When isResponsive={true} (scroll mode), the table will silently overflow its container on narrow screens — there's nothing to enable horizontal scrolling. The PR description promises "horizontal scroll instead," but that half was never implemented. Adding overflow-x: auto to TableWrapper is safe unconditionally (scroll only activates when content overflows). Fix this →

💡 Suggestions (non-blocking)

  • Inverted prop semantics (Table.tsx:965): isResponsive={false} (default) → data-responsive-mode="list", which IS the responsive behavior. isResponsive={true}data-responsive-mode="scroll", which opts out of it. The changeset example <Table isResponsive> also reads as "make this table responsive," yet it disables responsive layout. Consider mobileLayout?: 'list' | 'scroll' or keeping the original disableMobileListView name for clarity.

  • Inconsistent CSS attribute quote style: The [data-responsive-mode=...] selector alternates between single ('list') and double ("list") quotes — e.g. TableRow/StyledColGroup/StyledThead use single, TableData/MobileHeader/SelectData/ActionsList use double. Pick one and be consistent.

  • No test coverage: Table.test.tsx has no assertions for isResponsive. A test verifying data-responsive-mode="list" vs "scroll" is set on the outer container would be quick to add and guard against regressions.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 0s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Nice approach using a data-* attribute as a styling hook — that's the right pattern for this repo, and migrating from 768px to the theme breakpoint is a good improvement. The changeset is present with a usage example. ✅

One functional gap still needs to be addressed before merging:

🚨 Must Fix

  • TableWrapper missing overflow-x: auto (Table.tsx:501): TableWrapper has overflow: visible. When isResponsive={true} (scroll mode), the mobile list-view styles are correctly suppressed, but there's nothing enabling horizontal scroll — the table will silently overflow its container on narrow screens. The PR description explicitly promises "horizontal scroll instead." Adding overflow-x: auto to TableWrapper unconditionally is safe since scroll only activates when content overflows. Fix this →

💡 Suggestions (non-blocking)

  • Inverted prop semantics (Table.tsx:968): isResponsive={false} (the default) produces data-responsive-mode="list" — which IS the responsive behavior. isResponsive={true} disables it. This is counterintuitive; future consumers will likely expect isResponsive={true} to mean "this adapts to mobile." A union prop like mobileLayout?: 'list' | 'scroll' or keeping the original disableMobileListView name would make intent unambiguous.

  • Inconsistent CSS attribute quote style: Some selectors use single quotes ([data-responsive-mode='list'], e.g. lines 354, 398, 413) and others use double quotes ([data-responsive-mode="list"], e.g. lines 386, 426, 449). Both are valid CSS — pick one throughout.

  • No tests or stories: Per conventions, the new isResponsive behavior should have a test asserting the data-responsive-mode attribute value, and a Storybook story demonstrating the scroll-mode layout.

  • Pre-existing: ActionsList still has a hardcoded border-left: 1px solid (line 481) instead of ${theme.click.table.cell.stroke} — not introduced by this PR, but worth a follow-up.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 19s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Good pattern overall — using data-responsive-mode as a CSS styling hook is the right approach for this repo. The changeset is well-formed and the theme breakpoint migration is a nice improvement. Two issues need addressing before merge:

🚨 Must Fix

  • isResponsive logic is inverted vs. documentation (Table.tsx:968): The code is data-responsive-mode={isResponsive ? 'list' : 'scroll'}, which maps isResponsive={true}"list" (the old mobile list-view behavior). But the changeset, Storybook story description, and PR description all say isResponsive={true} should enable horizontal scroll (no mobile list view). This is a direct code/docs mismatch — one of them is wrong. The Storybook story also confirms the intent: isResponsive: true is the default arg, and the description reads "maintains standard layout with horizontal scroll." Fix by swapping the ternary: data-responsive-mode={isResponsive ? 'scroll' : 'list'}. Fix this →

  • TableWrapper still has overflow: visible (Table.tsx:504): When the mobile list-view styles are suppressed (scroll mode), there's nothing enabling horizontal scroll — the table will silently overflow its container on narrow screens. The Storybook story papers over this by wrapping in <div style={{ overflow: 'auto' }}>, but that's an undocumented requirement for consumers. Add overflow-x: auto to TableWrapper unconditionally (safe — scroll only activates when content overflows). Fix this →

💡 Suggestions (non-blocking)

  • Inconsistent CSS attribute quote style: [data-responsive-mode='list'] (single quotes, e.g. TableRow, StyledColGroup, StyledThead) and [data-responsive-mode="list"] (double quotes, e.g. TableData, MobileHeader, SelectData) are mixed. Both are valid CSS — pick one and be consistent.

  • No test coverage: Table.test.tsx has no assertions for isResponsive. A quick test verifying data-responsive-mode="list" vs "scroll" on the outer container would guard against regressions (and catch the inversion bug above at CI time).

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 24s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Good pattern and solid approach using data-responsive-mode as a CSS hook. The changeset is present and well-formed. The theme breakpoint migration is a clean improvement. Two blocking issues remain open from previous rounds of review — both need to land before merging.

🚨 Must Fix

  • Inverted ternary AND wrong default (Table.tsx:771,968): The code has isResponsive = true as the default and data-responsive-mode={isResponsive ? 'list' : 'scroll'}. This maps isResponsive={true}'list' (mobile list view ON), directly contradicting the changeset, PR description, and Storybook story description which all say isResponsive={true} should produce horizontal scroll (no mobile list view). The fix is two lines:

    1. Change default to isResponsive = false
    2. Swap the ternary to data-responsive-mode={isResponsive ? 'scroll' : 'list'}

    Result: default (false) → 'list' (unchanged old behavior), isResponsive={true}'scroll' (matching all docs). Fix this →

  • TableWrapper still has overflow: visible (Table.tsx:504): When isResponsive={true} suppresses the mobile list-view styles, nothing enables horizontal scroll — the table silently overflows its container on narrow screens. The Storybook story papers over this by wrapping in <div style={{ overflow: 'auto' }}>, but that's an undocumented consumer requirement. Adding overflow-x: auto to TableWrapper unconditionally is safe (scroll only activates when content overflows). Fix this →

💡 Suggestions (non-blocking)

  • Inconsistent CSS attribute quote style: [data-responsive-mode='list'] (single quotes, e.g. TableRow line 354, StyledColGroup line 398, StyledThead line 413) and [data-responsive-mode="list"] (double quotes, e.g. TableData line 386, MobileHeader line 426, SelectData line 449) are mixed. Both are valid CSS — pick one throughout.

  • No test coverage: Table.test.tsx has no assertions for isResponsive. A quick test verifying data-responsive-mode="list" vs "scroll" on the outer container would catch the inversion bug above at CI time and guard against future regressions.

  • Pre-existing: ActionsList has a hardcoded border-left: 1px solid (line 481) instead of ${theme.click.table.cell.stroke} — not introduced by this PR, worth a follow-up issue.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 51s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Good progress overall — the data-responsive-mode hook pattern is correct, the changeset describes the API accurately, and the theme breakpoint migration is clean. Note: previous automated reviews flagged the ternary at line 968 as "inverted," but it is actually correctisResponsive={true} (default) maps to 'list' (mobile list view on), isResponsive={false} maps to 'scroll' (mobile list view off). This is consistent with both changeset files and the story description.

One functional issue still needs to be addressed before merging:

Must Fix

  • TableWrapper still has overflow: visible (Table.tsx:504): When isResponsive={false}, the mobile list-view styles are correctly suppressed, but no horizontal scroll is enabled — the table will silently overflow its container on narrow screens. The Storybook story reveals this by working around it with div style={{ overflow: 'auto' }} (story lines 548, 551), but consumers would have no way to know they need this wrapper. Add overflow-x: auto to TableWrapper — safe unconditionally since scroll only activates when content overflows.

Suggestions (non-blocking)

  • Duplicate changeset files: Both .changeset/fuzzy-laws-know.md and .changeset/shy-toes-feel.md describe the same minor change with nearly identical content. Remove one before merging to avoid a double entry in the changelog.

  • Inconsistent CSS attribute quote style: [data-responsive-mode='list'] (single quotes, e.g. lines 354, 398, 413, 433, 492, 520) and [data-responsive-mode="list"] (double quotes, e.g. lines 386, 426, 449, 471) are mixed throughout. Both are valid CSS — pick one.

  • No test coverage: Table.test.tsx has no assertions for isResponsive. A quick test verifying the data-responsive-mode attribute value on the outer container would guard against regressions.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 21s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Good iteration — the changeset semantics are now correct and consistent with the implementation, and the Storybook story docs match the code. Two issues remain before merging.

🚨 Must Fix

  • Duplicate changeset files: Both .changeset/fuzzy-laws-know.md and .changeset/shy-toes-feel.md describe the exact same change. This will produce two separate changelog entries and a double minor version bump. Delete one of them. Fix this →

  • TableWrapper missing overflow-x: auto (Table.tsx:504): TableWrapper still has overflow: visible. The Storybook Responsive story works around this by wrapping the table in <div style={{ overflow: 'auto' }}> — but that's an undocumented requirement that consumers won't know about. When isResponsive={false} (scroll mode), the table will silently overflow its container on narrow screens with no horizontal scrollbar. Add overflow-x: auto to TableWrapper unconditionally (safe — scroll only activates when content overflows). Fix this →

💡 Suggestions (non-blocking)

  • Inconsistent CSS attribute quote style: [data-responsive-mode='list'] (single quotes — TableRow, StyledColGroup, StyledThead, Tbody, ActionsContainer, MobileActions, StyledTable) and [data-responsive-mode="list"] (double quotes — TableData, MobileHeader, SelectData, ActionsList) are mixed throughout the file. Both are valid CSS but pick one for consistency.

  • No tests: Table.test.tsx has no assertions for isResponsive. A quick test verifying the data-responsive-mode attribute on the outer container would prevent regressions.

  • Pre-existing: ActionsList still has a hardcoded border-left: 1px solid (line 481) instead of ${theme.click.table.cell.stroke} — not introduced by this PR, worth a follow-up issue.

…nce scroll only activates when content overflows
@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 3s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Good progress! The latest commit (b764371d) resolved the long-standing overflow-x: auto issue — TableWrapper at line 504 now correctly enables horizontal scroll unconditionally. The changeset content and implementation are now consistent: isResponsive={true} (default) → 'list' (mobile list view on), isResponsive={false}'scroll' (horizontal scroll mode). ✅

One blocking issue remains before merge:

🚨 Must Fix

  • Duplicate changeset files: Both .changeset/fuzzy-laws-know.md and .changeset/shy-toes-feel.md describe the exact same minor change. This will produce two changelog entries and a double minor version bump on release. Delete one. Fix this →

💡 Suggestions (non-blocking)

  • Inconsistent CSS attribute quote style: [data-responsive-mode='list'] (single quotes — lines 354, 398, 413, 433, 492, 520, 1162) and [data-responsive-mode="list"] (double quotes — lines 386, 426, 449, 471) are mixed throughout the file. Both are valid CSS; pick one for consistency.

  • Prop semantics are counterintuitive: isResponsive={true} (the default) enables the mobile list view — which is the adaptive/responsive behavior. isResponsive={false} opts into scroll-only — arguably the less responsive behavior. A future reader would likely expect isResponsive={true} to mean "this adapts to viewport width." Consider mobileLayout?: 'list' | 'scroll' for unambiguous intent, or at minimum a JSDoc comment on the prop explaining the inversion.

  • No test coverage: Table.test.tsx has no assertions for isResponsive. A quick test verifying data-responsive-mode attribute value on the outer container would guard against regressions.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 22s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

All previously flagged blockers have been resolved in this round of commits. The PR is in good shape. 🙌

Resolved since last review:

  • TableWrapper now has overflow-x: auto unconditionally (commit b764371d)
  • ✅ Duplicate changeset removed — only fuzzy-laws-know.md remains
  • ✅ CSS attribute quote style is now consistent throughout (all single quotes)
  • ✅ Changeset accurately documents the API (isResponsive={false} → scroll mode)

💡 Suggestions (non-blocking)

  • Story default is unintuitive: The Responsive story defaults to isResponsive: true (list view — the old behavior), while its description text explains isResponsive={false} (the new scroll behavior). A user opening this story in Storybook sees the old behavior by default and has to toggle the control to see what the story is actually about. Consider defaulting to isResponsive: false with maxWidth: '400px' so the scroll behavior is immediately visible.

  • No test coverage: Table.test.tsx has no assertions for isResponsive. A quick test verifying data-responsive-mode="list" vs "scroll" on the outer container would guard against regressions.

  • Prop semantics remain inverted (per previous reviews): isResponsive={true} enables the responsive/adaptive list view; isResponsive={false} opts into fixed scroll. Future readers will likely expect the inverse. This is now well-documented in the changeset, so it's workable — but worth a JSDoc comment on the prop for IDE discoverability.


No blockers remain. ✅

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 1m 19s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

Great iteration! All blocking issues from the previous review rounds are now resolved. Here's the current state:

✅ Fixed

  • TableWrapper now has overflow-x: auto (line 504) — horizontal scroll works correctly
  • Duplicate changeset file removed — only fuzzy-laws-know.md remains
  • CSS attribute selector quotes are now consistent (all single-quoted [data-responsive-mode='list'])
  • Tests added: Table.test.tsx lines 128–138 cover both isResponsive={true} (default list mode) and isResponsive={false} (scroll mode)
  • Changeset accurately documents the behavior: default isResponsive={true} → list mode, isResponsive={false} → scroll mode

💡 Suggestions (non-blocking)

  • Prop semantics are counterintuitive: isResponsive={true} (default) enables the mobile list view — which is the adaptive/responsive behavior. isResponsive={false} opts into scroll-only mode. A future reader would likely expect isResponsive={true} to mean "use the responsive layout" and isResponsive={false} to mean "disable it." Consider mobileLayout?: 'list' | 'scroll' or a JSDoc comment to make the inversion explicit:

    /** When false, disables mobile list view and uses horizontal scroll instead. Defaults to true (list view enabled). */
    isResponsive?: boolean;
  • Storybook story default arg: The Responsive story defaults to isResponsive: true (list mode), which doesn't showcase the new feature out of the box. Setting isResponsive: false as the default arg would make the scroll behavior the first thing viewers see, better communicating what the prop adds.

  • Pre-existing: ActionsList has a hardcoded border-left: 1px solid (line 481) instead of a theme token — not introduced by this PR, worth a follow-up.

This is ready to merge in my view — the implementation is sound, tests are in place, docs are accurate. 🚀

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 57s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

All the blocking issues from previous review rounds are resolved. This is in good shape to merge. 🙌

✅ Resolved

  • TableWrapper has overflow-x: auto — horizontal scroll works correctly
  • No duplicate changeset files (only fuzzy-laws-know.md)
  • CSS attribute selector quotes are consistent (all single-quoted [data-responsive-mode='list'])
  • Tests cover both modes (isResponsive={true}"list", isResponsive={false}"scroll")
  • Changeset accurately documents the API and includes a usage example
  • JSDoc on the prop explains the inversion clearly

💡 Suggestions (non-blocking)

  • Responsive story default is unintuitive (Table.stories.tsx:536): The story defaults to isResponsive: true (list mode — the old behavior), so a Storybook user opening it won't see the new feature. The story description explains isResponsive={false}, but the default arg shows the opposite. Defaulting to isResponsive: false with maxWidth: '400px' would make the scroll behavior immediately visible and better communicate what the prop adds.

  • Prop semantics remain inverted: isResponsive={true} (default) = mobile list view ON, isResponsive={false} = scroll only. The JSDoc comment does explain this, so it's workable. Worth a follow-up to consider mobileLayout?: 'list' | 'scroll' if/when the API is revisited.

  • Pre-existing: ActionsList has border-left: 1px solid (line 481) with a hardcoded 1px instead of ${theme.click.table.cell.stroke} — not introduced by this PR, worth a follow-up issue.

No blockers. ✅

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Claude finished @punkbit's task in 40s —— View job


Tasks

  • Gather context
  • Read changed files
  • Analyze conventions and guidelines
  • Provide review feedback

All previously flagged blockers are now resolved. The PR is in great shape. 🙌

✅ Resolved

  • TableWrapper now has overflow-x: auto (line 504) — horizontal scroll works correctly
  • Duplicate changeset removed — only fuzzy-laws-know.md remains
  • CSS attribute selector quotes are consistent (all single-quoted [data-responsive-mode='list'])
  • Tests added at Table.test.tsx:128–138 covering both isResponsive={true} (list mode) and isResponsive={false} (scroll mode)
  • Changeset accurately documents the API with correct default behavior
  • Story default is isResponsive: false with maxWidth: 400px, showing the scroll behavior out-of-the-box

💡 Suggestions (non-blocking)

  • Prop semantics (Table.tsx:583): isResponsive={true} (default) enables the mobile list view — which is the adaptive behavior. isResponsive={false} opts into fixed horizontal scroll. The JSDoc comment on the prop now documents this inversion, which is workable. A mobileLayout?: 'list' | 'scroll' union could make intent unambiguous for future consumers, but that's a nice-to-have.

  • Pre-existing: ActionsList has a hardcoded border-left: 1px solid (line 481) instead of ${theme.click.table.cell.stroke} — not introduced by this PR, worth a follow-up issue.

No blockers remain. ✅ Ready to merge.

@workflow-authentication-public
Copy link
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-9d2mfwf7o-clickhouse.vercel.app

Built from commit: cbe79dfdea76efa31eb810e60fc484119ec00705

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants