-
Notifications
You must be signed in to change notification settings - Fork 0
Create label atom #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
36187ad
6c159b6
13f82b7
50013d7
86ff40c
2d2a78a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import {render, screen} from "@testing-library/react"; | ||
| import {describe, expect, it} from "vitest"; | ||
| import {Label} from "./Label"; | ||
|
|
||
| describe("Label Atom", () => { | ||
| it("renders the label text correctly", () => { | ||
| render(<Label htmlFor="email">Email Address</Label>); | ||
|
|
||
| expect(screen.getByText("Email Address")).toBeInTheDocument(); | ||
| }); | ||
|
Comment on lines
+6
to
+10
|
||
|
|
||
| it("applies the htmlFor attribute correctly", () => { | ||
| render(<Label htmlFor="email">Email</Label>); | ||
|
|
||
| const label = screen.getByText("Email"); | ||
| expect(label).toHaveAttribute("for", "email"); | ||
| }); | ||
|
|
||
| it("shows required indicator when required prop is true", () => { | ||
| render( | ||
| <Label htmlFor="email" required> | ||
| </Label> | ||
| ); | ||
|
|
||
| expect(screen.getByText("*")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("does not show required indicator when required is false", () => { | ||
| render(<Label htmlFor="email">Email</Label>); | ||
|
|
||
| expect(screen.queryByText("*")).not.toBeInTheDocument(); | ||
| }); | ||
| }); | ||
|
Comment on lines
+5
to
+34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add missing test coverage for variants, disabled state, default props, and accessibility. Per coding guidelines, component tests must include coverage for default props, variants, disabled state, and accessibility. The Label component has a 📋 Suggested additional testsAdd the following test cases to ensure complete coverage: it("should apply default size (md) when no size prop is provided", () => {
render(<Label htmlFor="email">Email</Label>);
const label = screen.getByRole('label', { name: /email/i });
expect(label).toHaveClass('text-sm'); // md size maps to text-sm
});
it("should apply small size styling", () => {
render(<Label htmlFor="email" size="sm">Email</Label>);
const label = screen.getByRole('label', { name: /email/i });
expect(label).toHaveClass('text-xs');
});
it("should apply large size styling", () => {
render(<Label htmlFor="email" size="lg">Email</Label>);
const label = screen.getByRole('label', { name: /email/i });
expect(label).toHaveClass('text-base');
});
it("should apply disabled styling when disabled prop is true", () => {
render(<Label htmlFor="email" disabled>Email</Label>);
const label = screen.getByRole('label', { name: /email/i });
expect(label).toHaveClass('opacity-50');
expect(label).toHaveClass('cursor-not-allowed');
});
it("should apply custom className", () => {
render(<Label htmlFor="email" className="custom-class">Email</Label>);
const label = screen.getByRole('label', { name: /email/i });
expect(label).toHaveClass('custom-class');
});Based on coding guidelines. 🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| // src/components/atoms/Label/Label.tsx | ||
| import type {LabelProps} from "./LabelProps"; | ||
|
|
||
| const sizeClasses: Record<NonNullable<LabelProps["size"]>, string> = { | ||
| sm: "text-xs", | ||
| md: "text-sm", | ||
| lg: "text-base", | ||
| }; | ||
|
|
||
| /** | ||
| * A form label component that supports various sizes, required indicators, and disabled states. | ||
| * @see LabelProps | ||
| * @example | ||
| * <Label htmlFor="email" size="lg"> | ||
| * Email Address | ||
| * </Label> | ||
| */ | ||
| export default function Label({ | ||
| htmlFor, | ||
| children, | ||
| required, | ||
| size = "md", | ||
| disabled, | ||
| className, | ||
| }: Readonly<LabelProps>) { | ||
| return ( | ||
| <label | ||
| htmlFor={htmlFor} | ||
| className={[ | ||
| "block font-medium text-slate-700 dark:text-slate-300", | ||
| sizeClasses[size], | ||
| disabled && "opacity-50 cursor-not-allowed", | ||
| className, | ||
| ] | ||
| .filter(Boolean) | ||
| .join(" ")} | ||
| > | ||
| {children} | ||
|
|
||
| {required && ( | ||
| <> | ||
| {/* Screen readers only */} | ||
| <span className="sr-only">(required)</span> | ||
|
|
||
| {/* Visual required indicator */} | ||
| <span | ||
| aria-hidden="true" | ||
| className="ml-1 text-red-500" | ||
| > | ||
| * | ||
| </span> | ||
| </> | ||
|
|
||
|
|
||
| )} | ||
| </label> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // src/components/atoms/Label/LabelProps.ts | ||
| import type {ReactNode} from "react"; | ||
|
|
||
| export type LabelSize = "sm" | "md" | "lg"; | ||
|
|
||
| export interface LabelProps { | ||
| /** id of the input this label is associated with */ | ||
| htmlFor?: string; | ||
|
|
||
| /** label text */ | ||
| children: ReactNode; | ||
|
|
||
| /** show required asterisk */ | ||
| required?: boolean; | ||
|
|
||
| /** visual size */ | ||
| size?: LabelSize; | ||
|
|
||
| /** disabled appearance */ | ||
| disabled?: boolean; | ||
|
|
||
| /** extra tailwind classes */ | ||
| className?: string; | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,56 @@ | ||||||||||||||||||||||||||||||||||
| import { render, screen, fireEvent } from "@testing-library/react"; | ||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Import Per coding guidelines, use As per coding guidelines, use @testing-library/user-event for user interactions in tests. ♻️ Update imports-import { render, screen, fireEvent } from "@testing-library/react";
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
import { describe, it, expect, vi } from "vitest";
import Header from "./Header";
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||||||||||
| import { describe, it, expect, vi } from "vitest"; | ||||||||||||||||||||||||||||||||||
| import Header from "./Header"; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| import Header from "./Header"; | |
| import { Header } from "./Header"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Use "should" prefix for test names.
Per coding guidelines, test names should be descriptive and start with "should".
As per coding guidelines, use descriptive test names starting with 'should'.
♻️ Update test names
- it("renders the navbar container", () => {
+ it("should render the navbar container", () => {
render(<Header {...defaultProps} />);
expect(screen.getByRole("banner")).toBeInTheDocument();
});
- it("renders the app title", () => {
+ it("should render the app title", () => {
render(<Header {...defaultProps} />);
expect(screen.getByText("daisyUI")).toBeInTheDocument();
});
- it("renders the hamburger menu link", () => {
+ it("should render the hamburger menu link", () => {
render(<Header {...defaultProps} />);
const menuLink = screen.getAllByRole("link")[0];
expect(menuLink).toHaveAttribute("href", "/");
});
- it("renders profile button", () => {
+ it("should render profile button", () => {
render(<Header {...defaultProps} />);
expect(screen.getByRole("button")).toBeInTheDocument();
});
- it("shows the first letter of the user's name in profile image", () => {
+ it("should show the first letter of the user's name in profile image", () => {
render(<Header {...defaultProps} />);
const profileImg = screen.getByAltText("Karuna profile");
expect(profileImg).toHaveAttribute(
"src",
expect.stringContaining("K")
);
});
- it("uses fallback text when user is undefined", () => {
+ it("should use fallback text when user is undefined", () => {
render(<Header onLogout={mockLogout} />);
expect(screen.getByAltText("User profile")).toBeInTheDocument();
});
- it("calls onLogout when profile button is clicked", () => {
+ it("should call onLogout when profile button is clicked", async () => {
render(<Header {...defaultProps} />);
const profileButton = screen.getByRole("button");
- fireEvent.click(profileButton);
+ await userEvent.click(profileButton);
expect(mockLogout).toHaveBeenCalledOnce();
});Also applies to: 18-21, 23-27, 29-32, 34-41, 43-46, 48-55
🤖 Prompt for AI Agents
In @src/components/layout/Header/Header.test.tsx around lines 13 - 16, Rename
the test titles to start with "should" and make them descriptive: e.g., change
it("renders the navbar container", ...) to it("should render the navbar
container", ...) and similarly update the other test descriptions around the
Header component tests that call render(<Header {...defaultProps} />) and
assertions like expect(screen.getByRole("banner")).toBeInTheDocument(); and the
tests that check links, buttons, and conditional items; keep the test bodies
unchanged, only update the string descriptions for lines mentioned (and the
other ranges) so all test names begin with "should".
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the project's testing standards, test descriptions should start with "should" for consistency. For example: "should render the navbar container" instead of "renders the navbar container". This pattern is established in the coding guidelines and helps maintain a consistent AAA (Arrange-Act-Assert) testing pattern throughout the project.
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using getAllByRole with array indexing [0] is fragile and can lead to brittle tests. Instead, use a more specific query that uniquely identifies the hamburger menu link, such as checking for the specific link that contains the SVG icon or using a more descriptive role query. Consider using getByRole('link', { name: /menu/i }) if the link has accessible text, or add a test-id if necessary as a last resort.
| const menuLink = screen.getAllByRole("link")[0]; | |
| const menuLink = screen.getByRole("link", { name: /daisyUI/i }); |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the project's coding guidelines, fireEvent should be replaced with userEvent from '@testing-library/user-event' for more realistic user interaction simulation. The guidelines specifically state "Use @testing-library/user-event for user interactions" and "Always use async/await with user events".
The code should be:
const user = userEvent.setup();
await user.click(profileButton);Note that the test function would also need to be made async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Use userEvent.click() with async/await instead of fireEvent.click().
Per coding guidelines, always use async/await with user events. Replace fireEvent.click() with userEvent.click() for more realistic user interaction simulation.
As per coding guidelines, always use async/await with user events in tests.
♻️ Refactor to use userEvent
- it("calls onLogout when profile button is clicked", () => {
+ it("calls onLogout when profile button is clicked", async () => {
render(<Header {...defaultProps} />);
const profileButton = screen.getByRole("button");
- fireEvent.click(profileButton);
+ await userEvent.click(profileButton);
expect(mockLogout).toHaveBeenCalledOnce();
});📝 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.
| it("calls onLogout when profile button is clicked", () => { | |
| render(<Header {...defaultProps} />); | |
| const profileButton = screen.getByRole("button"); | |
| fireEvent.click(profileButton); | |
| expect(mockLogout).toHaveBeenCalledOnce(); | |
| }); | |
| it("calls onLogout when profile button is clicked", async () => { | |
| render(<Header {...defaultProps} />); | |
| const profileButton = screen.getByRole("button"); | |
| await userEvent.click(profileButton); | |
| expect(mockLogout).toHaveBeenCalledOnce(); | |
| }); |
🤖 Prompt for AI Agents
In @src/components/layout/Header/Header.test.tsx around lines 48 - 55, The test
should use async/await with userEvent instead of fireEvent: make the test
callback async, replace fireEvent.click(profileButton) with await
userEvent.click(profileButton), and ensure userEvent is imported (or available)
in Header.test.tsx so the user interaction is simulated realistically when
asserting mockLogout in the "calls onLogout when profile button is clicked"
test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Use "should" prefix for test names and semantic queries.
Test names should start with "should" per coding guidelines. Additionally, prefer semantic queries like
getByLabelTextorgetByRoleovergetByTextfor better accessibility testing.As per coding guidelines, test names should be descriptive and start with 'should'.
♻️ Proposed refactor with semantic queries and proper naming
Also applies to: 12-17, 19-27, 29-33
🤖 Prompt for AI Agents