feat(counter): add counter application with increment, decrement and reset - #2
vidyashreebv wants to merge 5 commits into
Conversation
| onClick={onClick} | ||
| disabled={disabled} | ||
| aria-label={ariaLabel} | ||
| className={clsx( |
There was a problem hiding this comment.
Good use of clsx here.
Can you define it at the top and assign it as a variable here? it gives more readability and clean-code.
There was a problem hiding this comment.
Great start with test cases, we'll plan more learning in testing in future.
| aria-label={label ? `${label}: ${value}` : `counter value: ${value}`} | ||
| > |
There was a problem hiding this comment.
const cardAriaLabel = label ? `${label}: ${value}` : `counter value: ${value}`we can use this variable for "aria-label" attribute.
|
|
||
| <div className="flex gap-4 mb-6"> | ||
| <Button | ||
| onClick={decrement} |
There was a problem hiding this comment.
these are handle functions, we have give a convention in function namings
| const increment = () => setCount(count + step) | ||
| const decrement = () => setCount(count - step) | ||
| const reset = () => setCount(initialValue) |
There was a problem hiding this comment.
Handle click function should have function conventions.
| const increment = () => setCount(count + step) | |
| const decrement = () => setCount(count - step) | |
| const reset = () => setCount(initialValue) | |
| const handleClickIncrement = () => setCount(count + step) | |
| const handleClickDecrement = () => setCount(count - step) | |
| const handleClickReset = () => setCount(initialValue) |
| </div> | ||
|
|
||
| <Button | ||
| onClick={reset} |
There was a problem hiding this comment.
| onClick={reset} | |
| onClick={() => setCount(initialValue)} |
since it's one-liner this could work.
| import Button from '../../atoms/Button/Button' | ||
| import Display from '../../atoms/Display/Display' | ||
|
|
||
| const Counter = ({ initialValue = 0, step = 1 }) => { |
There was a problem hiding this comment.
named export are recommended.
|
@cw-pr-agent review |
Change SummaryThis PR implements a Counter feature composed of new Button and Display atomic components and a Counter organism. It integrates the Counter into App with a green gradient theme, updates Tailwind import, and adds unit tests plus a Testing Library devDependency. Note: PR description mentions "27 tests total" but the diff includes three new test files (Button, Display, Counter); additional tests may exist outside shown diffs. 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}
| const disabledStyles = 'opacity-50 cursor-not-allowed' | ||
|
|
||
| return ( | ||
| <button |
There was a problem hiding this comment.
Severity: 🟠 Major
Add an explicit type="button" default and guard unknown variant values to avoid accidental form submits and silent styling failures.
Suggested Change:
const Button = ({
children,
onClick,
variant = 'primary',
disabled = false,
ariaLabel,
className = '',
type = 'button',
}) => {
// ...
const variantClass = variants[variant] ?? variants.primary
return (
<button
type={type}
onClick={onClick}
disabled={disabled}
aria-label={ariaLabel}
className={clsx(baseStyles, variantClass, disabled && disabledStyles, className)}
>
{children}
</button>
)
}
| const Counter = ({ initialValue = 0, step = 1 }) => { | ||
| const [count, setCount] = useState(initialValue) | ||
|
|
||
| const increment = () => setCount(count + step) |
There was a problem hiding this comment.
Severity: 🟠 Major
Use functional state updates (and handle* naming) so rapid/batched updates can't apply stale count values.
Suggested Change:
const handleIncrement = () => setCount((current) => current + step)
const handleDecrement = () => setCount((current) => current - step)
const handleReset = () => setCount(initialValue)
// ...
<Button onClick={handleDecrement} ... />
<Button onClick={handleIncrement} ... />
<Button onClick={handleReset} ... />
| expect(handleClick).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('renders with primary variant by default', () => { |
There was a problem hiding this comment.
Severity: 🔴 Critical
Fix the default-variant assertion to match the component styles and prefer role/name queries for resilient, a11y-aligned tests.
Suggested Change:
it('renders with primary variant by default', () => {
render(<Button>Click me</Button>)
const button = screen.getByRole('button', { name: /click me/i })
expect(button).toHaveClass('bg-green-600')
})
| expect(screen.getByText('10')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders without label', () => { |
There was a problem hiding this comment.
Severity: 🟠 Major
Replace the invalid queryByRole('paragraph') check with an assertion that actually verifies no label element is rendered.
Suggested Change:
it('renders without label', () => {
const { container } = render(<Display value={0} />)
expect(container.querySelector('p')).toBeNull()
expect(screen.getByText('0')).toBeInTheDocument()
})
PR OverviewPR Type: Feature Focus Areas for Architect ReviewConfirm the Tailwind CSS v4 Decide whether Consider standardizing atom APIs (e.g., forwarding native button props and PR InsightsPotential PR Improvements
Strengths
|
vidya-cw
added
the
Closed and Can't merge
4 participants
What does this PR do?
What steps must a reviewer take to test the PR manually?
npm installnpm run dev- verify app starts at http://localhost:5173npm run test- verify all 27 tests passScreenshots
Definition of done Checklist
Code Quality
Testing