fix: Persist settings panel preferences to localStorage#31
fix: Persist settings panel preferences to localStorage#31
Conversation
Connect SettingsPanel inputs to useSettingsStore: - Import useSettingsStore hook for state management - Convert Weekly Hours Target to controlled input with onChange - Convert Timesheet Reminders to controlled checkbox with onChange - Settings now persist via Zustand's persist middleware to localStorage Fixes #10
There was a problem hiding this comment.
Pull request overview
This PR connects the SettingsPanel component to the useSettingsStore hook to persist user preferences to localStorage using Zustand's persist middleware.
- Imports useSettingsStore and extracts weeklyHoursTarget, notificationsEnabled, and updateSettings
- Converts the Weekly Hours Target input from uncontrolled (defaultValue) to controlled (value + onChange)
- Converts the Timesheet Reminders checkbox from uncontrolled (defaultChecked) to controlled (checked + onChange)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| onChange={(e) => | ||
| updateSettings({ | ||
| weeklyHoursTarget: parseInt(e.target.value, 10) || 0, | ||
| }) | ||
| } |
There was a problem hiding this comment.
The fallback value of 0 when parsing fails could result in an invalid weeklyHoursTarget. When a user clears the input field or enters invalid text, parseInt returns NaN, which would then be replaced with 0. This means users cannot intentionally clear the field, and it will immediately snap to 0, which may not be the desired behavior. Consider either maintaining the previous valid value when parsing fails, or using an empty string check before parsing to preserve user input during editing.
| onChange={(e) => | |
| updateSettings({ | |
| weeklyHoursTarget: parseInt(e.target.value, 10) || 0, | |
| }) | |
| } | |
| onChange={(e) => { | |
| const parsed = parseInt(e.target.value, 10); | |
| updateSettings({ | |
| weeklyHoursTarget: Number.isNaN(parsed) ? weeklyHoursTarget : parsed, | |
| }); | |
| }} |
| <input | ||
| type="number" | ||
| defaultValue={40} | ||
| value={weeklyHoursTarget} | ||
| onChange={(e) => | ||
| updateSettings({ | ||
| weeklyHoursTarget: parseInt(e.target.value, 10) || 0, | ||
| }) | ||
| } | ||
| min={0} | ||
| max={168} | ||
| className="w-20 rounded-lg border border-dark-600 bg-dark-800 px-3 py-2 text-dark-100 focus:outline-none focus:ring-2 focus:ring-thyme-500" | ||
| /> |
There was a problem hiding this comment.
The number input lacks an accessible label. The nearby text "Weekly Hours Target" should be associated with this input using either an id and htmlFor pairing or by wrapping the input in a <label> element. Screen readers currently won't announce the purpose of this input when users focus on it.
| <label className="relative inline-flex cursor-pointer items-center"> | ||
| <input type="checkbox" defaultChecked className="peer sr-only" /> | ||
| <input | ||
| type="checkbox" | ||
| checked={notificationsEnabled} | ||
| onChange={(e) => updateSettings({ notificationsEnabled: e.target.checked })} | ||
| className="peer sr-only" | ||
| /> | ||
| <div className="peer h-6 w-11 rounded-full bg-dark-600 after:absolute after:left-[2px] after:top-[2px] after:h-5 after:w-5 after:rounded-full after:bg-white after:transition-all after:content-[''] peer-checked:bg-thyme-600 peer-checked:after:translate-x-full peer-focus:outline-none peer-focus:ring-2 peer-focus:ring-thyme-500"></div> | ||
| </label> |
There was a problem hiding this comment.
The checkbox input lacks an accessible label. While the input is wrapped in a <label> element, it doesn't contain any text content that screen readers can announce. The nearby text "Timesheet Reminders" should be included within the label element or associated with the input using id and htmlFor attributes so screen readers can properly announce the checkbox's purpose.
Connect SettingsPanel inputs to useSettingsStore:
Fixes #10