fix: Persist Settings Panel preferences using Zustand store#23
fix: Persist Settings Panel preferences using Zustand store#23
Conversation
Connect SettingsPanel form inputs to useSettingsStore hook so that user preferences (weekly hours target, notifications) are saved to localStorage and persist across sessions. Fixes #10
There was a problem hiding this comment.
Pull request overview
This PR enhances the SettingsPanel component by integrating it with a Zustand-based persistence layer, enabling user preferences for weekly hours target and notifications to be saved to localStorage and persist across browser sessions.
- Connects form inputs to the
useSettingsStorehook for state management - Converts uncontrolled inputs (
defaultValue,defaultChecked) to controlled inputs (value,checked) - Adds validation logic to ensure weekly hours target stays within the valid range (1-168 hours)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type="checkbox" | ||
| checked={notificationsEnabled} | ||
| onChange={(e) => updateSettings({ notificationsEnabled: e.target.checked })} | ||
| className="peer sr-only" |
There was a problem hiding this comment.
The custom checkbox toggle is missing an accessible label association. While there is descriptive text next to the toggle, screen readers cannot determine what the checkbox controls. Add an aria-label or aria-labelledby attribute to the checkbox input to associate it with the "Timesheet Reminders" label for better accessibility.
| className="peer sr-only" | |
| className="peer sr-only" | |
| aria-label="Timesheet Reminders" |
| <input | ||
| type="number" | ||
| defaultValue={40} | ||
| value={weeklyHoursTarget} | ||
| min={1} | ||
| max={168} | ||
| onChange={(e) => { | ||
| const value = parseInt(e.target.value, 10); | ||
| if (!isNaN(value) && value >= 1 && value <= 168) { | ||
| updateSettings({ weeklyHoursTarget: value }); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The number input is missing a 'step' attribute. While the validation ensures only integers between 1 and 168 are accepted, without an explicit step attribute, some browsers may allow decimal inputs via the input spinner buttons. Consider adding step={1} to ensure the input only accepts whole numbers through all input methods.
| <input | ||
| type="number" | ||
| defaultValue={40} | ||
| value={weeklyHoursTarget} | ||
| min={1} | ||
| max={168} | ||
| onChange={(e) => { | ||
| const value = parseInt(e.target.value, 10); | ||
| if (!isNaN(value) && value >= 1 && value <= 168) { | ||
| updateSettings({ weeklyHoursTarget: value }); | ||
| } | ||
| }} | ||
| 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 is missing an accessible label association. While there is descriptive text next to the input, screen readers cannot determine what the input controls. Add an aria-label or aria-labelledby attribute to the input element to associate it with the "Weekly Hours Target" label for better accessibility.
|
Closing in favour of #30 which is more complete. See knowall-ai/thyme-bc-extension#4 for the planned approach to fetch hours target from BC Resources API. |
Connect SettingsPanel form inputs to useSettingsStore hook so that
user preferences (weekly hours target, notifications) are saved to
localStorage and persist across sessions.
Fixes #10