-
-
Notifications
You must be signed in to change notification settings - Fork 6
feat: Improve calendar UI and widen desktop layout for collaborative workflows #356
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 all commits
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 |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ import { MapToggle } from './map-toggle' | |
| import { ProfileToggle } from './profile-toggle' | ||
|
|
||
| export const Header = () => { | ||
| const { toggleCalendar } = useCalendarToggle() | ||
| const { toggleCalendar, isCalendarOpen } = useCalendarToggle() | ||
| return ( | ||
| <header className="fixed w-full p-1 md:p-2 flex justify-between items-center z-10 backdrop-blur md:backdrop-blur-none bg-background/80 md:bg-transparent"> | ||
| <div> | ||
|
|
@@ -38,7 +38,7 @@ export const Header = () => { | |
|
|
||
| <MapToggle /> | ||
|
|
||
| <Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar"> | ||
| <Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar" className={cn(isCalendarOpen && "bg-accent")}> | ||
| <CalendarDays className="h-[1.2rem] w-[1.2rem]" /> | ||
| </Button> | ||
|
Comment on lines
+41
to
43
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. Good call adding an active visual state. However, the button should expose its toggle state to assistive tech and be labeled accurately when open/closed. Consider adding This improves accessibility and clarity without affecting the visual design intent. SuggestionUpdate the button to reflect toggle state and improve contrast: <Button
variant="ghost"
size="icon"
onClick={toggleCalendar}
title={isCalendarOpen ? "Close Calendar" : "Open Calendar"}
aria-pressed={isCalendarOpen}
aria-label={isCalendarOpen ? "Close calendar" : "Open calendar"}
className={cn(isCalendarOpen && "bg-accent text-accent-foreground")}
>
<CalendarDays className="h-[1.2rem] w-[1.2rem]" />
</Button>Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion. |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ interface MobileIconsBarProps { | |
| export const MobileIconsBar: React.FC<MobileIconsBarProps> = ({ onAttachmentClick }) => { | ||
| const [, setMessages] = useUIState<typeof AI>() | ||
| const { clearChat } = useActions() | ||
| const { toggleCalendar } = useCalendarToggle() | ||
| const { toggleCalendar, isCalendarOpen } = useCalendarToggle() | ||
|
Contributor
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. 🧹 Nitpick | 🔵 Trivial Consider using The active state styling works correctly, but Apply this diff to align with the pattern in - <Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar" className={isCalendarOpen ? "bg-accent" : ""}>
+ <Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar" className={cn(isCalendarOpen && "bg-accent")}>
<CalendarDays className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" />
</Button>You'll also need to import import { cn } from '@/lib/utils'Also applies to: 45-45 🤖 Prompt for AI Agents |
||
|
|
||
| const handleNewChat = async () => { | ||
| setMessages([]) | ||
|
|
@@ -42,7 +42,7 @@ export const MobileIconsBar: React.FC<MobileIconsBarProps> = ({ onAttachmentClic | |
| <CircleUserRound className="h-[1.2rem] w-[1.2rem]" /> | ||
| </Button> | ||
| <MapToggle /> | ||
| <Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar"> | ||
| <Button variant="ghost" size="icon" onClick={toggleCalendar} title="Open Calendar" className={isCalendarOpen ? "bg-accent" : ""}> | ||
| <CalendarDays className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" /> | ||
| </Button> | ||
|
Comment on lines
+45
to
47
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. Nice active-state addition. Similar to desktop, the button should communicate its state for accessibility and label accuracy. Add SuggestionImprove accessibility and labeling: <Button
variant="ghost"
size="icon"
onClick={toggleCalendar}
title={isCalendarOpen ? "Close Calendar" : "Open Calendar"}
aria-pressed={isCalendarOpen}
aria-label={isCalendarOpen ? "Close calendar" : "Open calendar"}
className={isCalendarOpen ? "bg-accent" : undefined}
>
<CalendarDays className="h-[1.2rem] w-[1.2rem] transition-all rotate-0 scale-100" />
</Button>Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion. |
||
| <Button variant="ghost" size="icon"> | ||
|
|
||
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.
With the right pane fixed at
w-2/5, increasing the left column tow-3/5is reasonable. However, flex items withoutmin-w-0can cause unexpected horizontal overflow when their children usew-full(as the notepad now does). Addingmin-w-0to the left column prevents long content from forcing overflow and improves layout stability across browsers.Suggestion
Add
min-w-0to the left column:Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.