-
Notifications
You must be signed in to change notification settings - Fork 0
fix(golf,baseball): calendar mobile clip + retired accent-stripe card #806
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,8 @@ import { | |
| AlertCircle, | ||
| } from 'lucide-react'; | ||
| import { MobileRSVPButtons, type RSVPResponse } from './MobileRSVPButtons'; | ||
| import { formatTime } from '@/lib/calendar/event-styles'; | ||
| import { formatTime, getEventTypeConfig } from '@/lib/calendar/event-styles'; | ||
| import type { EventType } from '@/lib/types/calendar'; | ||
| import type { CalendarEvent } from '@/hooks/useCalendarEvents'; | ||
| import { Button } from '@/components/ui/button'; | ||
|
|
||
|
|
@@ -91,17 +92,8 @@ export function MobileEventCard({ | |
| return onRsvp(response); | ||
| }, [onRsvp]); | ||
|
|
||
| // Get accent color based on event type (matches event-styles.ts) | ||
| const getAccentColor = () => { | ||
| switch (event.event_type) { | ||
| case 'practice': return 'bg-warm-400'; | ||
| case 'tournament': return 'bg-primary-600'; | ||
| case 'qualifier': return 'bg-amber-500'; | ||
| case 'meeting': return 'bg-sky-500'; | ||
| case 'travel': return 'bg-purple-500'; | ||
| default: return 'bg-warm-400'; | ||
| } | ||
| }; | ||
| // Cast string type to EventType for styling (fallback to 'other' if not matched) | ||
| const config = getEventTypeConfig(event.event_type as EventType); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When baseball calendar rows use known event types like Context Used: Review for Helm Sports Labs (a commercial multi-sp... (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: src/components/golf/calendar/MobileEventCard.tsx
Line: 96
Comment:
**Baseball Types Lose Styling**
When baseball calendar rows use known event types like `showcase` or `tryout`, this cast sends those strings through the shared golf/baseball styling lookup even though they are not in the shared `EventType` config. The new dot becomes the only visible event-type marker on mobile, so those events fall back to neutral `other` styling and become visually indistinguishable from miscellaneous events.
**Context Used:** Review for Helm Sports Labs (a commercial multi-sp... ([source](.greptile))
How can I resolve this? If you propose a fix, please make it concise.
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| return ( | ||
| <div | ||
|
|
@@ -115,12 +107,6 @@ export function MobileEventCard({ | |
| className | ||
| )} | ||
| > | ||
| {/* Left accent bar */} | ||
| <div className={cn( | ||
| 'absolute left-0 top-0 bottom-0 w-1 rounded-l-2xl', | ||
| getAccentColor() | ||
| )} /> | ||
|
|
||
| {/* Main card content - tappable */} | ||
| <Button variant="ghost" | ||
| type="button" | ||
|
|
@@ -154,10 +140,16 @@ export function MobileEventCard({ | |
| </div> | ||
| )} | ||
|
|
||
| {/* Title */} | ||
| <h3 className="text-subhead font-medium text-warm-900 leading-snug truncate"> | ||
| {event.title} | ||
| </h3> | ||
| {/* Title — leading ringed event-type dot (replaces the retired left accent stripe) */} | ||
| <div className="flex items-center gap-2"> | ||
| <span | ||
| className={cn('h-1.5 w-1.5 rounded-full shrink-0 ring-[3px]', config.dotColor, config.dotRingColor)} | ||
| aria-hidden="true" | ||
| /> | ||
| <h3 className="text-subhead font-medium text-warm-900 leading-snug truncate min-w-0"> | ||
| {event.title} | ||
| </h3> | ||
| </div> | ||
|
|
||
| {/* Time and location row */} | ||
| <div className="flex flex-wrap items-center gap-x-3 gap-y-1 mt-1.5"> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,10 +148,12 @@ export function NotificationCenter() { | |
|
|
||
| return ( | ||
| <div className="relative"> | ||
| {/* Bell Button */} | ||
| {/* Bell Button — overflow-visible overrides the Button primitive's | ||
| ripple-containing overflow-hidden, which was clipping the | ||
| -top-1/-right-1 unread badge below. */} | ||
| <Button variant="ghost" | ||
| onClick={handleToggle} | ||
| className="relative p-2.5 rounded-xl bg-cream-100/75 backdrop-blur-sm border border-warm-200/45 shadow-sm text-warm-500 hover:text-warm-800 hover:bg-cream-50/92 hover:shadow-md active:scale-95 transition-all duration-200" | ||
| className="relative overflow-visible p-2.5 rounded-xl bg-cream-100/75 backdrop-blur-sm border border-warm-200/45 shadow-sm text-warm-500 hover:text-warm-800 hover:bg-cream-50/92 hover:shadow-md active:scale-95 transition-all duration-200" | ||
| aria-label={unreadCount > 0 ? `Notifications, ${unreadCount} unread` : 'Notifications'} | ||
| aria-expanded={isOpen} | ||
| > | ||
|
Comment on lines
154
to
159
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3. Bell icon button lacks tooltip The icon-only bell Button has an aria-label but is not wrapped in a tooltip component. This fails the requirement that icon-only buttons provide both a screen-reader label and a visible tooltip. Agent Prompt
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'primary', | ||
| bgColor: 'bg-primary-50/60', | ||
| dotColor: 'bg-primary-500', | ||
| dotRingColor: 'ring-primary-500/[0.18]', | ||
| textColor: 'text-primary-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -20,6 +21,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'primary', | ||
| bgColor: 'bg-primary-50/60', | ||
| dotColor: 'bg-primary-600', | ||
| dotRingColor: 'ring-primary-600/[0.18]', | ||
| textColor: 'text-primary-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -29,6 +31,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'amber', | ||
| bgColor: 'bg-amber-50/60', | ||
| dotColor: 'bg-amber-500', | ||
| dotRingColor: 'ring-amber-500/[0.18]', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Keep the added ring classes on the approved design-token palette. The new styles introduce As per coding guidelines, use only canonical color families: Also applies to: 44-44, 54-54, 64-64, 74-74, 84-84, 94-94, 104-104, 114-114, 124-156 🤖 Prompt for AI AgentsSource: Coding guidelines
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skipped deliberately: the amber/stone/teal/violet/orange/sky/rose/purple families are pre-existing in Generated by Claude Code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| textColor: 'text-amber-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -38,6 +41,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'stone', | ||
| bgColor: 'bg-stone-100/60', | ||
| dotColor: 'bg-stone-400', | ||
| dotRingColor: 'ring-stone-400/[0.18]', | ||
| textColor: 'text-stone-700', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -47,6 +51,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'teal', | ||
| bgColor: 'bg-teal-50/60', | ||
| dotColor: 'bg-teal-500', | ||
| dotRingColor: 'ring-teal-500/[0.18]', | ||
| textColor: 'text-teal-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -56,6 +61,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'violet', | ||
| bgColor: 'bg-violet-50/60', | ||
| dotColor: 'bg-violet-500', | ||
| dotRingColor: 'ring-violet-500/[0.18]', | ||
| textColor: 'text-violet-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -65,6 +71,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'orange', | ||
| bgColor: 'bg-orange-50/60', | ||
| dotColor: 'bg-orange-500', | ||
| dotRingColor: 'ring-orange-500/[0.18]', | ||
| textColor: 'text-orange-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -74,6 +81,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'sky', | ||
| bgColor: 'bg-sky-50/60', | ||
| dotColor: 'bg-sky-500', | ||
| dotRingColor: 'ring-sky-500/[0.18]', | ||
| textColor: 'text-sky-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -83,6 +91,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'rose', | ||
| bgColor: 'bg-rose-50/60', | ||
| dotColor: 'bg-rose-500', | ||
| dotRingColor: 'ring-rose-500/[0.18]', | ||
| textColor: 'text-rose-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -92,6 +101,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'stone', | ||
| bgColor: 'bg-stone-100/50', | ||
| dotColor: 'bg-stone-300', | ||
| dotRingColor: 'ring-stone-300/[0.18]', | ||
| textColor: 'text-stone-500', | ||
| showText: false, | ||
| }, | ||
|
|
@@ -101,6 +111,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'stone', | ||
| bgColor: 'bg-stone-100/40', | ||
| dotColor: 'bg-stone-200', | ||
| dotRingColor: 'ring-stone-200/[0.18]', | ||
| textColor: 'text-stone-400', | ||
| showText: false, | ||
| }, | ||
|
|
@@ -110,6 +121,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'purple', | ||
| bgColor: 'bg-purple-50/60', | ||
| dotColor: 'bg-purple-500', | ||
| dotRingColor: 'ring-purple-500/[0.18]', | ||
| textColor: 'text-purple-800', | ||
| showText: true, | ||
| }, | ||
|
|
@@ -119,6 +131,7 @@ const eventTypeConfigs: Record<EventType, EventTypeConfig> = { | |
| color: 'stone', | ||
| bgColor: 'bg-stone-100/60', | ||
| dotColor: 'bg-stone-400', | ||
| dotRingColor: 'ring-stone-400/[0.18]', | ||
| textColor: 'text-stone-600', | ||
| showText: true, | ||
| }, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Import
EventTypethrough the public type barrel.Use
import type { EventType } from '@/lib/types'instead of the deep@/lib/types/calendarimport.As per coding guidelines, import types from
@/lib/typesfor type definitions.🤖 Prompt for AI Agents
Source: Coding guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipped deliberately:
@/lib/types/index.tsdoes not re-export the calendar module, so the barrel import would not compile. The deep@/lib/types/calendarpath is the established idiom for these types — the siblingEventCard.tsx(pre-existing) importsEventTypethe same way. Expanding the barrel is out of scope for this fix (69 existing exports, collision risk with inline calendar-event types already in index.ts).Generated by Claude Code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 6102
@njrini99-codeConfirmed —src/lib/types/index.tshas no re-export of./calendar, andsrc/components/golf/calendar/EventCard.tsx:8already importsEventTypethe same way (from '@/lib/types/calendar'). The barrel import would indeed fail to compile as-is, and this is the established pattern for these types. Withdrawing this nit.🧬 Code Graph Analysis Results
Relevant snippets
src/components/golf/calendar/MobileEventCard.tsxtype { EventType }from@/lib/types/calendarand castingevent.event_type as EventType.)src/lib/types/calendar.ts(lines 5-20)src/lib/calendar/event-styles.ts(lines 165-172)src/lib/calendar/event-styles.ts(lines 177-196)src/hooks/useCalendarEvents.ts(lines 6-34)src/components/golf/calendar/MobileRSVPButtons.tsx(line 20)src/lib/utils.ts(lines 5-7)