[REQ-002] Render 30-day heatmap and weekly stats label in UI#11
Conversation
Each habit card now shows a 30-cell CSS-grid heatmap (last 30 days, today rightmost) and an "X / 7 this week" label. Cells use isCompletedOn per day; weekly count uses last7Count. Exports shiftDay from habits.ts to avoid duplicating date arithmetic in main.ts. Closes #6
feffa1c to
b04c32e
Compare
markoub
left a comment
There was a problem hiding this comment.
Review: PR #11 — 30-day heatmap & weekly stats
This is a clean, well-scoped implementation. All seven acceptance criteria from issue #6 are satisfied. Here's the detailed breakdown:
✅ Correctness vs acceptance criteria
| Criterion | Verdict |
|---|---|
| 30-cell CSS-grid heatmap, today = rightmost | ✅ shiftDay(today, i - 29) for i 0 → 29 correctly places the oldest day at index 0 and today at index 29 |
| Completed vs not-completed cells visually distinct | ✅ .heatmap-cell.done { background: var(--accent) } vs muted #2c2f50 |
"X / 7 this week" label driven by last7Count |
✅ src/main.ts:69 |
No inline date arithmetic duplicated in main.ts |
✅ All date logic goes through shiftDay / isCompletedOn from habits.ts |
Each cell has title with ISO date |
✅ title="${day}" on every div.heatmap-cell |
| Layout doesn't break at ≥ 320 px | ✅ grid-template-columns: repeat(30, 1fr) lets cells scale down gracefully |
npm test and npm run build pass |
✅ CI check test completed with success |
✅ Purity of src/habits.ts
The only change to habits.ts is promoting shiftDay from a module-private function to an export (src/habits.ts:49). No DOM, no localStorage, no window, no Date.now() without a parameter — pure as required.
✅ Test coverage
Four new tests for shiftDay in src/habits.test.ts:51–68:
- Forward shift (+1 day)
- Backward shift (−1 day, crossing a month boundary)
- Zero delta (identity)
- Month boundary (May→June, June back 29 days)
The cases cover the meaningful edge cases for this helper. Good work.
✅ CSS fix for .habits li.empty
Moving align-items: center from the base .habits li rule to the .habits li.empty override (src/style.css:130) is exactly right: the base li now uses flex-direction: column, so align-items: center would mis-align the habit row. The empty placeholder still centres its single text child correctly.
Non-blocking nit
PR title / commit format — the title [REQ-002] Render 30-day … doesn't follow the project's conventional-commit convention (feat: prefix). Not a blocker here since the squash-merge commit title can be set to feat: render 30-day heatmap and weekly stats (#11), but worth noting for future PRs.
Observation (not a blocker) — the .heatmap div carries aria-hidden="true" (src/main.ts:72), which means screen readers will never reach the title attributes on individual cells. The acceptance criterion asks only that title attributes exist (for hover tooltips), so this is technically satisfied. The weekly stats text is visible to screen readers, which provides a useful summary. No change required, but consider whether a visually-hidden summary span inside the heatmap might be a worthwhile future enhancement.
Verdict
Meets the bar. CI is green, the PR is mergeable, purity is intact, tests cover the new export, and all acceptance criteria are satisfied. Labelling ready-to-merge and squash-merging now.
Generated by Claude Code
Summary
shiftDayfromsrc/habits.tsso date arithmetic stays in one place (no duplication inmain.ts).var(--accent); empty days use the muted background. Each cell carries atitleattribute with its ISO date for accessibility.last7Count(habit, today).grid-template-columns: repeat(30, 1fr)so cells shrink gracefully on narrow screens (≥ 320 px).lirefactored toflex-direction: columnwith an inner.habit-rowdiv for the check/name/streak/delete row.How I tested
shiftDayhelper (forward, backward, zero delta, month-boundary).npm test— all 31 tests pass.npm run build— TypeScript type-check clean, Vite build succeeds.Closes #6