[Feat]: add streak milestone celebration banner (issue #225)#331
[Feat]: add streak milestone celebration banner (issue #225)#331ap-ananya wants to merge 4 commits into
Conversation
|
@ap-ananya is attempting to deploy a commit to the PRIYANSHU DOSHI's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thanks for your first PR on DevTrack! 🎉
A maintainer will review it within 48 hours. While you wait:
- Make sure CI is passing (type-check + lint)
- Double-check the PR description is filled out and the issue is linked
- Feel free to ask questions in Discussions if you need help
Priyanshu-byte-coder
left a comment
There was a problem hiding this comment.
Milestone detection and localStorage persistence logic are correct. Three quick fixes:
1. Hardcoded color: 'white' in StreakMilestoneBanner
// Current
style={{ background: 'var(--accent)', color: 'white' }}
// Fix — accent-foreground is always readable on top of accent
style={{ background: 'var(--accent)', color: 'var(--accent-foreground)' }}2. Missing aria-label on dismiss button
<button onClick={() => onDismiss?.()} aria-label='Dismiss milestone banner'>
✕
</button>3. Unnecessary whitespace-only changes in dashboard/page.tsx and page.tsx — please revert those lines, they add noise to the diff.
Fix these three and this is ready to merge.
|
Good milestone celebration feature — localStorage persistence, milestone array, dismiss logic. A few fixes: 1. Hardcoded style={{ background: "var(--accent)", color: "var(--accent-foreground)" }}2. Missing <button type="button" onClick={() => onDismiss?.()} aria-label="Dismiss milestone banner">3. Add try/catch around try {
setDismissedMilestones(JSON.parse(stored));
} catch { /* ignore */ }Fix these three and push — will merge. |
|
Addressed all review feedback:
Thanks for the review!! |
Priyanshu-byte-coder
left a comment
There was a problem hiding this comment.
Logic and localStorage pattern are correct. Three small fixes needed:
1. Move milestones to module level
const milestones = [7, 30, 50, 100, 200, 365] is a static constant — re-created on every render inside the component. Move it outside:
// top of file, before the component
const STREAK_MILESTONES = [7, 30, 50, 100, 200, 365];2. Dismiss button has no visible styling
The ✕ button has no className — on the var(--accent) background it looks unstyled and has a tiny click target. Add at minimum:
<button
type="button"
onClick={() => onDismiss?.()}
aria-label="Dismiss milestone banner"
className="ml-4 rounded-md p-1 opacity-70 hover:opacity-100 transition-opacity"
>
✕
</button>3. Minor: extra blank line in page.tsx + missing newline at EOF in StreakMilestoneBanner.tsx
The page.tsx change adds an empty line that wasn't there. Revert that. Add a newline at the end of StreakMilestoneBanner.tsx (missing \n after the last }).
Everything else looks good — correct useEffect pattern for localStorage, CSS vars used throughout, aria-label on the dismiss button, proper fragment wrapping.
|
Addressed latest review feedback:
Thanks again for the review! |
Summary
Added a streak milestone celebration banner for milestone streaks.
Closes #225
Features Implemented
StreakMilestoneBannercomponentHow to Test
✕) and confirm the banner disappearsTesting
UI Update (Test image)
Checklist