feat(card): add accessible card component - #7
LikithaYadavG wants to merge 2 commits into
Conversation
|
@cw-pr-agent review |
Change SummaryThis PR adds a reusable, accessible Card component and its sub-components (CardHeader, CardBody, CardFooter, CardTitle, CardDescription, CardImage). The Card supports static, clickable (button), and link (anchor) modes, includes keyboard focus styles for accessibility, and exposes variant and padding options. A comprehensive Vitest + React Testing Library test suite verifies rendering, interactivity, variants, padding, and accessibility behaviors. File Changes
|
PR ScorecardScoreScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
|
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| @@ -0,0 +1,213 @@ | |||
| import type { | |||
There was a problem hiding this comment.
Severity: 🟠 Major
Avoid relying on the global React namespace for types; import the needed types explicitly for consistency and stricter TS configs.
Suggested Change:
import type {
AnchorHTMLAttributes,
ButtonHTMLAttributes,
HTMLAttributes,
ImgHTMLAttributes,
MouseEventHandler,
ReactNode,
} from "react";
export type CardBaseProps = {
// ...
children?: ReactNode;
};
type CardImageProps = {
src: string;
alt: string;
className?: string;
} & Omit<ImgHTMLAttributes<HTMLImageElement>, "src" | "alt" | "className">;
| children?: React.ReactNode; | ||
| }; | ||
|
|
||
| export type ClickableCardProps = CardBaseProps & |
There was a problem hiding this comment.
Severity: 🟠 Major
Prevent consumers from overriding type="button" and make prop precedence explicit by spreading buttonProps first and omitting type from the public props.
Why this matters: A Card used inside a <form> can unexpectedly submit if type is overridden to submit.
Suggested Change:
export type ClickableCardProps = CardBaseProps &
Omit<ButtonHTMLAttributes<HTMLButtonElement>, "onClick" | "type"> & {
onClick: MouseEventHandler<HTMLButtonElement>;
href?: never;
"aria-label"?: string;
};
// ...
<button
{...buttonProps}
type="button"
onClick={onClick}
aria-label={ariaLabel}
className={`${baseStyles} text-left w-full ${focusStyles}`}
>
{children}
</button>
| const focusStyles = | ||
| "focus:outline-none focus-visible:ring-2 focus-visible:ring-blue-500 focus-visible:ring-offset-2"; | ||
|
|
||
| export const Card = ({ |
There was a problem hiding this comment.
Severity: 🟠 Major
Document that interactive Cards render a native <button>/<a> and therefore must not contain nested interactive elements.
Why this matters: Nested buttons/links/inputs inside a button/anchor are invalid HTML and commonly break screen readers and keyboard interaction.
Suggested Change:
/**
* When `onClick` is provided, Card renders a <button>; when `href` is provided, Card renders an <a>.
* Avoid nesting interactive elements (buttons, links, inputs) inside interactive Cards.
*/
export const Card = ({
variant = "elevated",
hover = false,
padding = "none",
className = "",
children,
...restProps
}: CardProps) => {
// ...
};
|
|
||
| const isInteractive = isClickable || isLink; | ||
|
|
||
| const baseStyles = [ |
There was a problem hiding this comment.
Severity: 💬 Minor [nitpick]
Disable hover affordances when the clickable Card is disabled, so visuals match behavior.
Suggested Change:
const isClickable = "onClick" in restProps && typeof restProps.onClick === "function";
const isLink = "href" in restProps && typeof restProps.href === "string";
const isDisabled =
isClickable && "disabled" in restProps && Boolean((restProps as { disabled?: boolean }).disabled);
const baseStyles = [
"rounded-lg transition-all",
variantStyles[variant],
paddingStyles[padding],
hover && isInteractive && !isDisabled && "hover:shadow-lg hover:-translate-y-0.5",
className,
]
.filter(Boolean)
.join(" ");
| ); | ||
| }; | ||
|
|
||
| export const CardHeader = ({ |
There was a problem hiding this comment.
Severity: 💬 Minor [nitpick]
Extract the repeated section padding into a single constant so CardHeader/Body/Footer and CardImage stay in sync.
Suggested Change:
const SECTION_PADDING = "px-6 py-4";
export const CardHeader = ({ className = "", children, ...props }: HTMLAttributes<HTMLElement>) => (
<header
className={`${SECTION_PADDING} border-b border-gray-200 ${className}`}
{...props}
>
{children}
</header>
);
export const CardBody = ({ className = "", children, ...props }: HTMLAttributes<HTMLDivElement>) => (
<div className={`${SECTION_PADDING} ${className}`} {...props}>
{children}
</div>
);
export const CardFooter = ({ className = "", children, ...props }: HTMLAttributes<HTMLElement>) => (
<footer
className={`${SECTION_PADDING} border-t border-gray-200 ${className}`}
{...props}
>
{children}
</footer>
);
| ).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("should call onClick when button card is clicked", async () => { |
There was a problem hiding this comment.
Severity: 🟠 Major
Add keyboard interaction tests (Tab + Enter/Space) for interactive cards to match the PR’s stated accessibility requirements.
Suggested Change:
it("should be focusable via Tab and activate on Enter for clickable card", async () => {
const user = userEvent.setup();
const onClick = vi.fn();
renderCard({
additionalProps: {
onClick,
"aria-label": "Clickable card",
},
});
await user.tab();
expect(screen.getByRole("button", { name: /clickable card/i })).toHaveFocus();
await user.keyboard("{Enter}");
expect(onClick).toHaveBeenCalledTimes(1);
});
it("should be focusable via Tab when rendered as a link", async () => {
const user = userEvent.setup();
renderCard({ additionalProps: { href: "/test-link" } });
await user.tab();
expect(screen.getByRole("link", { name: /card content/i })).toHaveFocus();
});
| expect(screen.getByRole("article")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("should apply elevated variant styles by default", () => { |
There was a problem hiding this comment.
Severity: 🟠 Major
Avoid asserting implementation-specific Tailwind classes; add stable data-* attributes and assert those instead.
Why this matters: It keeps tests resilient to style refactors while still validating the chosen variant/padding.
Suggested Change:
// Card.tsx: add stable attributes on the rendered interactive element
// static:
<article className={baseStyles} data-variant={variant} data-padding={padding} {...staticProps}>
// clickable: (on <button>)
<button ... data-variant={variant} data-padding={padding}>
// link: (on <a>)
<a ... data-variant={variant} data-padding={padding}>// Card.test.tsx: assert data attributes instead of classnames
it("should apply elevated variant by default", () => {
renderCard();
expect(screen.getByRole("article")).toHaveAttribute("data-variant", "elevated");
});
it("should apply outlined variant", () => {
renderCard({ additionalProps: { variant: "outlined" } });
expect(screen.getByRole("article")).toHaveAttribute("data-variant", "outlined");
});
it("should apply padding when provided", () => {
renderCard({ additionalProps: { padding: "md" } });
expect(screen.getByRole("article")).toHaveAttribute("data-padding", "md");
});
PR InsightsPotential PR Improvements
Strengths
|
| type RenderOptions = { | ||
| additionalProps?: CardProps; | ||
| }; |
There was a problem hiding this comment.
we don't have to create it as separate type.
| render( | ||
| <Card> | ||
| <CardHeader>Header</CardHeader> | ||
| </Card>, | ||
| ); |
There was a problem hiding this comment.
check the possible way to render the children also in the renderCard function itself
| additionalProps: { | ||
| variant: "outlined", | ||
| }, |
There was a problem hiding this comment.
it could be directly a variant instead of additionalProps:{} and sending.
| it("should apply outlined variant styles", () => { | ||
| renderCard({ | ||
| additionalProps: { | ||
| variant: "outlined", | ||
| }, | ||
| }); | ||
|
|
||
| expect(screen.getByRole("article")).toHaveClass("border-2"); | ||
| }); |
| <header | ||
| className={`px-6 py-4 border-b border-gray-200 ${className}`} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </header> |
| type CardImageProps = { | ||
| src: string; | ||
| alt: string; | ||
| className?: string; | ||
| } & Omit< | ||
| React.ImgHTMLAttributes<HTMLImageElement>, | ||
| "src" | "alt" | "className" | ||
| >; |
There was a problem hiding this comment.
remove this omits. and check the source type itself.
|
Tip Need another review? Tag me and say rereview for re-analysis after you have fixed all the issues. @cw-pr-agent rereview |
2 participants
PR Template & Definition of Done
What does this PR do?
What steps does your reviewer have to take to test this PR manually?
import { Card, CardBody, CardTitle } from "@/components/card/Card"<Card variant="outlined"><CardBody>Content</CardBody></Card><Card onClick={handleClick} hover>Click me</Card><Card href="/page" hover>Navigate</Card>Pull Request standards checklist - Please check off
Testing checklist - Please check off
If you have not followed and completed any of the above, please explain why below.
N/A
Definition of Done - Please check off