feat(customization): add settings panel for initial value and step configuration - #3
vidyashreebv wants to merge 2 commits into
Conversation
| <input | ||
| type="number" | ||
| value={initialValue} | ||
| onChange={(e) => setInitialValue(Number(e.target.value))} |
There was a problem hiding this comment.
use full naming everywhere
| onChange={(e) => setInitialValue(Number(e.target.value))} | |
| onChange={(event) => setInitialValue(Number(event.target.value))} |
| </div> | ||
| <div className="flex items-end"> | ||
| <button | ||
| onClick={handleApply} |
There was a problem hiding this comment.
| onClick={handleApply} | |
| onClick={handleClickApply} |
|
Good work!! Thanks for the detailed PR description and review guidelines. I've reviewed all PRs, and here are a few common feedback points:
|
|
@cw-pr-agent review |
Change SummaryThis PR adds a settings panel above the counter that lets users configure the counter's initial value and step increment/decrement. It implements state hooks for these settings and an "Apply" button that re-initializes the Counter by updating a key to force remount. It also introduces a centralized theme constants file for design tokens used across the UI. 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}
| import Counter from './components/organisms/Counter/Counter' | ||
|
|
||
| function App() { | ||
| const [initialValue, setInitialValue] = useState(10) |
There was a problem hiding this comment.
Severity: 🔴 Critical
Parsing type="number" input with Number(e.target.value) on every keystroke breaks intermediate states (e.g. -, empty, 1.) and can set NaN; keep draft values as strings and parse/validate on Apply.
Suggested Change:
const DEFAULT_INITIAL_VALUE = 10
const DEFAULT_STEP = 5
const [draftInitialValue, setDraftInitialValue] = useState(String(DEFAULT_INITIAL_VALUE))
const [draftStep, setDraftStep] = useState(String(DEFAULT_STEP))
const [{ initialValue, step }, setCounterSettings] = useState({
initialValue: DEFAULT_INITIAL_VALUE,
step: DEFAULT_STEP,
})
const [counterVersion, setCounterVersion] = useState(0)
const handleApply = () => {
const nextInitialValue = Number(draftInitialValue)
const nextStep = Number(draftStep)
if (!Number.isFinite(nextInitialValue) || !Number.isFinite(nextStep)) return
setCounterSettings({ initialValue: nextInitialValue, step: nextStep })
setCounterVersion(v => v + 1)
}
// ...
<input
type="number"
value={draftInitialValue}
onChange={(e) => setDraftInitialValue(e.target.value)}
step="any"
/>
<input
type="number"
value={draftStep}
onChange={(e) => setDraftStep(e.target.value)}
step="any"
/>
<Counter key={counterVersion} initialValue={initialValue} step={step} />
| <h3 className="text-lg font-bold text-gray-800 mb-3">Counter Settings</h3> | ||
| <div className="flex gap-3 flex-wrap"> | ||
| <div className="flex-1 min-w-[140px]"> | ||
| <label className="block text-xs font-medium text-gray-700 mb-1"> |
There was a problem hiding this comment.
Severity: 🟠 Major
Associate labels with inputs via htmlFor/id to improve accessibility and click/AT behavior.
Suggested Change:
<label
htmlFor="initialValue"
className="block text-xs font-medium text-gray-700 mb-1"
>
Initial Value
</label>
<input id="initialValue" /* ... */ />
<label
htmlFor="stepValue"
className="block text-xs font-medium text-gray-700 mb-1"
>
Step Value
</label>
<input id="stepValue" /* ... */ />
| function App() { | ||
| const [initialValue, setInitialValue] = useState(10) | ||
| const [step, setStep] = useState(5) | ||
| const [key, setKey] = useState(0) |
There was a problem hiding this comment.
Severity: 💬 Minor [nitpick]
Rename the key state to something explicit (e.g. counterVersion) to avoid confusion with React element keys and clarify intent.
Suggested Change:
const [counterVersion, setCounterVersion] = useState(0)
const handleApply = () => {
setCounterVersion(v => v + 1)
}
<Counter key={counterVersion} initialValue={initialValue} step={step} />
| /> | ||
| </div> | ||
| <div className="flex items-end"> | ||
| <button |
There was a problem hiding this comment.
Severity: 💬 Minor [nitpick]
Add type="button" to prevent accidental form submission if this UI is ever placed inside a <form>.
Suggested Change:
<button type="button" onClick={handleApply}>
Apply
</button>
| } | ||
|
|
||
| return ( | ||
| <div className="min-h-screen bg-gradient-to-br from-green-50 via-emerald-50 to-teal-50 flex items-center justify-center p-4"> |
There was a problem hiding this comment.
Severity: 🟠 Major
Add RTL tests covering settings changes + Apply + counter +/- + Reset to prevent regressions in the new core user flow.
Suggested Change:
// App.test.jsx (example)
import { render, screen } from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import App from './App'
test('applies initial value and step, and reset returns to applied initial value', async () => {
const user = userEvent.setup()
render(<App />)
await user.clear(screen.getByLabelText(/initial value/i))
await user.type(screen.getByLabelText(/initial value/i), '50')
await user.clear(screen.getByLabelText(/step value/i))
await user.type(screen.getByLabelText(/step value/i), '10')
await user.click(screen.getByRole('button', { name: /apply/i }))
// Assert the displayed count / behavior based on your Counter UI
})
| @@ -0,0 +1,40 @@ | |||
| /** | |||
There was a problem hiding this comment.
Severity: 🟠 Major
If theme tokens aren’t being used yet, consider removing this file from the PR; otherwise wire it into App.jsx/Counter to avoid dead code.
Suggested Change:
// Option A: remove this file until used
// Option B: import and use tokens where className strings are duplicated
// import { colors } from './constants/theme'
PR OverviewPR Type: Feature Focus Areas for Architect ReviewThe counter reset is implemented via a remount ( The current number input handling should support intermediate typing states ( No tests were added in the diff for the new settings/apply flow; confirm existing test coverage or request RTL tests that assert visible count changes and reset behavior. 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 installandnpm run devnpm run testScreenshots
Definition of done Checklist
Code Quality
Testing