Rebuild working hours calculator UI - #28
Conversation
There was a problem hiding this comment.
Pull request overview
This PR rebuilds the working hours calculator UI with a modernized design system and improved user experience. The changes transform a utility-focused interface into a more visually polished application while maintaining the core functionality of pattern creation, validation, and comparison.
Key Changes
- Replaced the simple header with a hero section featuring dynamic stat cards that reflect contract settings and pattern counts in real-time
- Implemented comprehensive pattern management with duplication, deletion, live validation, and visual feedback through gap badges
- Added localStorage persistence with automatic state recovery and introduced an "auto-sync" mode for target hours that keeps them aligned with weekly settings
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| index.html | Restructured layout with hero section, stat cards, improved semantic HTML structure, and template-based pattern rendering; added Google Fonts dependency |
| styles.css | Complete design system overhaul with new color scheme, modern gradients, enhanced spacing/typography, and responsive grid layouts |
| app.js | Refactored state management with improved localStorage handling, simplified calculation logic, template-based DOM manipulation, and new auto-target feature |
| </template> | ||
|
|
||
| <script type="module" src="./app.js"></script> | ||
| <script src="app.js"></script> |
There was a problem hiding this comment.
The script tag has been changed from type="module" to no type attribute. If ES6 module features are not needed, this is fine, but if any module-specific features like top-level imports/exports are used, this could cause issues. Verify that the JavaScript doesn't rely on module scope behavior.
| <script src="app.js"></script> | |
| <script type="module" src="app.js"></script> |
| } else { | ||
| week2 += result.hours; | ||
| } | ||
| } else if (day.start || day.end || day.breakMinutes !== '') { |
There was a problem hiding this comment.
The condition checks if day.breakMinutes !== '' but breakMinutes is stored as a number (line 239 sets it to a number). An empty input would be 0 or undefined, not an empty string. This condition may not work as intended for detecting incomplete entries. Consider checking for day.breakMinutes == null || day.breakMinutes === undefined instead, or ensure consistent type handling.
| } else if (day.start || day.end || day.breakMinutes !== '') { | |
| } else if (day.start || day.end || day.breakMinutes != null) { |
| ...state, | ||
| patterns: state.patterns.map((p) => (p.id === id ? { ...p, name } : p)), | ||
| return { | ||
| id: crypto.randomUUID ? crypto.randomUUID() : `pattern-${Date.now()}-${Math.random()}`, |
There was a problem hiding this comment.
The fallback ID generation pattern-${Date.now()}-${Math.random()} is unnecessarily complex. Since this fallback only applies if crypto.randomUUID is unavailable (very old browsers), consider using a simpler counter-based approach or accepting that this tool requires modern browser features.
| <template id="pattern-template"> | ||
| <article class="pattern-card"> | ||
| <header class="pattern-card__header"> | ||
| <input class="pattern-name" type="text" /> |
There was a problem hiding this comment.
The pattern name input lacks an accessible label. Screen reader users won't know what this field is for. Consider adding an aria-label attribute (e.g., aria-label="Pattern name") to provide context.
| <input class="pattern-name" type="text" /> | |
| <input class="pattern-name" type="text" aria-label="Pattern name" /> |
| const bestIndex = totals.reduce((best, current, idx) => { | ||
| if (best === -1) return idx; | ||
| const currentGap = Math.abs(current.gap); | ||
| const bestGap = Math.abs(totals[best].gap); | ||
| return currentGap < bestGap ? idx : best; | ||
| }, totals.length ? 0 : -1); |
There was a problem hiding this comment.
The bestIndex calculation doesn't filter out patterns with invalid days (hasInvalid flag). A pattern with invalid entries could be marked as "best match" if it happens to have a calculated gap closest to zero. Consider only comparing valid patterns when determining the best match, or at least documenting this behavior.
| const bestIndex = totals.reduce((best, current, idx) => { | |
| if (best === -1) return idx; | |
| const currentGap = Math.abs(current.gap); | |
| const bestGap = Math.abs(totals[best].gap); | |
| return currentGap < bestGap ? idx : best; | |
| }, totals.length ? 0 : -1); | |
| // Only consider valid patterns (hasInvalid !== true) for best match | |
| const validPatternIndices = totals | |
| .map((metrics, idx) => ({ metrics, idx })) | |
| .filter(({ metrics }) => !metrics.hasInvalid); | |
| let bestIndex = -1; | |
| if (validPatternIndices.length > 0) { | |
| const bestValid = validPatternIndices.reduce((best, current) => { | |
| const currentGap = Math.abs(current.metrics.gap); | |
| const bestGap = Math.abs(best.metrics.gap); | |
| return currentGap < bestGap ? current : best; | |
| }); | |
| bestIndex = bestValid.idx; | |
| } |
| } | ||
| breakInput.addEventListener('change', (event) => { | ||
| const value = Number(event.target.value); | ||
| pattern.days[dayIndex].breakMinutes = Number.isNaN(value) ? '' : value; |
There was a problem hiding this comment.
When the break input value is invalid (NaN), it's stored as an empty string. This is inconsistent with the number type expected for breakMinutes. Consider storing 0 or null for invalid values to maintain type consistency throughout the application.
| pattern.days[dayIndex].breakMinutes = Number.isNaN(value) ? '' : value; | |
| pattern.days[dayIndex].breakMinutes = Number.isNaN(value) ? 0 : value; |
| summary.innerHTML = ` | ||
| <div class="helper-row"><strong>Week 1:</strong> ${metrics.week1.toFixed(2)} hrs</div> | ||
| <div class="helper-row"><strong>Week 2:</strong> ${metrics.week2.toFixed(2)} hrs</div> | ||
| <div class="helper-row"><strong>Fortnight:</strong> ${metrics.fortnight.toFixed(2)} hrs</div> | ||
| <div class="helper-row"><strong>Gap:</strong> ${formatGap(metrics.gap)}</div> | ||
| `; |
There was a problem hiding this comment.
The summary is rendered using innerHTML with string concatenation of HTML structures. This creates a mix of DOM manipulation approaches (template cloning elsewhere vs innerHTML here). Consider using consistent DOM creation methods throughout for better maintainability.
| summary.innerHTML = ` | |
| <div class="helper-row"><strong>Week 1:</strong> ${metrics.week1.toFixed(2)} hrs</div> | |
| <div class="helper-row"><strong>Week 2:</strong> ${metrics.week2.toFixed(2)} hrs</div> | |
| <div class="helper-row"><strong>Fortnight:</strong> ${metrics.fortnight.toFixed(2)} hrs</div> | |
| <div class="helper-row"><strong>Gap:</strong> ${formatGap(metrics.gap)}</div> | |
| `; | |
| // Clear previous summary rows | |
| summary.innerHTML = ''; | |
| // Week 1 | |
| const week1Row = document.createElement('div'); | |
| week1Row.className = 'helper-row'; | |
| const week1Strong = document.createElement('strong'); | |
| week1Strong.textContent = 'Week 1:'; | |
| week1Row.appendChild(week1Strong); | |
| week1Row.appendChild(document.createTextNode(` ${metrics.week1.toFixed(2)} hrs`)); | |
| summary.appendChild(week1Row); | |
| // Week 2 | |
| const week2Row = document.createElement('div'); | |
| week2Row.className = 'helper-row'; | |
| const week2Strong = document.createElement('strong'); | |
| week2Strong.textContent = 'Week 2:'; | |
| week2Row.appendChild(week2Strong); | |
| week2Row.appendChild(document.createTextNode(` ${metrics.week2.toFixed(2)} hrs`)); | |
| summary.appendChild(week2Row); | |
| // Fortnight | |
| const fortnightRow = document.createElement('div'); | |
| fortnightRow.className = 'helper-row'; | |
| const fortnightStrong = document.createElement('strong'); | |
| fortnightStrong.textContent = 'Fortnight:'; | |
| fortnightRow.appendChild(fortnightStrong); | |
| fortnightRow.appendChild(document.createTextNode(` ${metrics.fortnight.toFixed(2)} hrs`)); | |
| summary.appendChild(fortnightRow); | |
| // Gap | |
| const gapRow = document.createElement('div'); | |
| gapRow.className = 'helper-row'; | |
| const gapStrong = document.createElement('strong'); | |
| gapStrong.textContent = 'Gap:'; | |
| gapRow.appendChild(gapStrong); | |
| gapRow.appendChild(document.createTextNode(` ${formatGap(metrics.gap)}`)); | |
| summary.appendChild(gapRow); |
| <td><input type="time" class="day-start" /></td> | ||
| <td><input type="time" class="day-end" /></td> | ||
| <td><input type="number" min="0" step="5" class="day-break" /></td> |
There was a problem hiding this comment.
The time and number inputs in the day row template lack accessible labels. Screen reader users navigating the table won't have clear context for these inputs beyond the column headers. Consider adding aria-label attributes to each input that combine the day name with the field purpose (e.g., "Monday start time", "Monday end time", "Monday break minutes"). These labels should be set dynamically in the JavaScript when the rows are created.
| const fortnight = Math.round((week1 + week2) * 100) / 100; | ||
| const gap = Math.round((fortnight - Number(settings.targetFortnightHours || 0)) * 100) / 100; | ||
|
|
||
| function duplicatePattern(id) { | ||
| const existing = state.patterns.find((p) => p.id === id); | ||
| if (!existing) return; | ||
| const copy = { | ||
| ...existing, | ||
| id: crypto.randomUUID(), | ||
| name: `${existing.name} (copy)`, | ||
| days: existing.days.map((d) => ({ ...d })), | ||
| return { | ||
| week1: Math.round(week1 * 100) / 100, | ||
| week2: Math.round(week2 * 100) / 100, |
There was a problem hiding this comment.
The fortnight value is calculated by rounding the sum of week1 and week2, but week1 and week2 are also rounded independently at lines 124-125. This could lead to minor discrepancies due to double rounding. Consider calculating fortnight directly from the sum before any rounding, then rounding only once, or document why this double-rounding approach is intentional.
| startInput.addEventListener('change', (event) => { | ||
| pattern.days[dayIndex].start = event.target.value; | ||
| saveAndRender(); | ||
| }); | ||
|
|
||
| card.appendChild(header); | ||
| const body = document.createElement("div"); | ||
| body.className = "panel-body"; | ||
| body.appendChild(tableWrapper); | ||
| card.appendChild(body); | ||
| container.appendChild(card); | ||
| }); | ||
| endInput.addEventListener('change', (event) => { | ||
| pattern.days[dayIndex].end = event.target.value; | ||
| saveAndRender(); | ||
| }); | ||
|
|
||
| bindPatternEvents(); | ||
| } | ||
|
|
||
| function formatGap(gap) { | ||
| const cls = gap === 0 ? "gap-zero" : gap > 0 ? "gap-positive" : "gap-negative"; | ||
| const sign = gap > 0 ? "+" : ""; | ||
| return `<span class="${cls}">${sign}${gap.toFixed(2)}</span>`; | ||
| } | ||
| breakInput.addEventListener('change', (event) => { | ||
| const value = Number(event.target.value); | ||
| pattern.days[dayIndex].breakMinutes = Number.isNaN(value) ? '' : value; | ||
| saveAndRender(); | ||
| }); |
There was a problem hiding this comment.
Each input change triggers a full re-render of all patterns via saveAndRender. While acceptable for a small number of patterns, this approach could become sluggish with many patterns (10+), especially since localStorage writes are synchronous. Consider debouncing the saves or using more targeted DOM updates if performance becomes an issue with multiple patterns.
Summary
Testing
Codex Task