Skip to content

test(CardHorizontal): 💍 visual regression#997

Open
punkbit wants to merge 27 commits intomainfrom
test/cardhorizontal-visual-regression
Open

test(CardHorizontal): 💍 visual regression#997
punkbit wants to merge 27 commits intomainfrom
test/cardhorizontal-visual-regression

Conversation

@punkbit
Copy link
Copy Markdown
Contributor

@punkbit punkbit commented Apr 15, 2026

Why?

Introduce visual regression tests for CardHorizontal, as a prerequisite for migrating styled components into CSS modules.

How?

  • Extend CardHorizontal storybook stories
  • Introduce CardHorizontal test spec for visual regression
  • Store snapshots

Contribution checklist?

  • You've done enough research before writing
  • You have reviewed the PR
  • The commit messages are detailed
  • The build command runs locally
  • Assets or static content are linked and stored in the project
  • For documentation, guides or references, you've tested the commands

Preview?

Screenshot 2026-04-15 at 13 31 11

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 15, 2026

⚠️ No Changeset found

Latest commit: 4830992

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@punkbit punkbit changed the title Test/cardhorizontal visual regression test(CardHorizontal): 💍 visual regression Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 15, 2026

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


PR Review

  • Read CLAUDE.md and conventions
  • Review changes since last review
  • Check if revert fixed the must-fix issue
  • Final assessment

The latest commit (48309927) reverts the story removal that was flagged as a must-fix in the previous review. Both MutedSelected and DefaultDisabledSelected stories are now present and correctly mapped to the spec tests.

✅ Previously flagged must-fix — now resolved

Item Status
MutedSelected story missing (stories.tsx:126) ✅ Restored by revert
DefaultDisabledSelected story missing (stories.tsx:136) ✅ Restored by revert
aria-pressed default (CardHorizontal.tsx:230) isSelected ?? false
isSelected=undefined unit test ✅ present
Space key handler (CardHorizontal.tsx:213) ✅ implemented & tested

💡 Pre-existing observations (non-blocking, carry-over from prior reviews)

Keyboard nav test presses Enter instead of Space (spec.ts:382) — the component only handles ' ' in handleKeyDown. The test passes silently but doesn't exercise the implemented behavior. Fix this →

Redundant isSelectable guard (CardHorizontal.tsx:214) — if (isSelectable && ...) is unreachable because onKeyDown is only attached when isSelectable is true (line 232). Harmless.

Double-fire on info button click (CardHorizontal.tsx:294–299) — inner <Button onClick={handleClick}> bubbles to outer <Wrapper onClick={handleClick}>. Pre-existing; worth a follow-up issue.


✅ All must-fix items from previous reviews are resolved — good to merge!

@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

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

Built from commit: b2d93315f84abeb705d9cde74bc0d2ce4a6286f7

@punkbit
Copy link
Copy Markdown
Contributor Author

punkbit commented Apr 15, 2026

  • CardHorizontal.tsx:293-300 — Pre-existing: inner <Button onClick={handleClick}> + outer <Wrapper onClick={handleClick}> will double-fire on button clicks due to bubbling. Worth a follow-up issue.

This is addressed in pending PR #998. Do ignore!

@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 15, 2026
@punkbit punkbit marked this pull request as ready for review April 15, 2026 15:29
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