Global Component Library - #199
Conversation
- new workspace package @sage/ui - package scaffold - wired main to package with workspace dependency - added .turbo to .gitignore
- added shared UI primitives in packages/ui/src - replaced main app imports from local to @sage/ui
- extracted common dev banner, logos, nav links, user profile - created routing hook
- update tailwind config to scan package - created package docs
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
joalen
left a comment
joalen
left a comment
There was a problem hiding this comment.
Great work and a solid job trying to make it globalized!! Code looks good, but some suggestions I have...
- We should globalize the sidebar; things that would differ when inherited from parent sidebar component: left button content + action, sidebar content, buttons related to collapsed state of sidebar and its actions. Everything else I think can be kept the same as far as I know.
- UI clippings for certain elements (check below):
(should be above the navbar...check reference image labeled below):
Reference image:
(sage logo not visible...check the reference img labeled below):
Reference image:
- extracted common collapse toggle, collapsed rail, green primary action button - planner adopts template - chatbot page wants to be weird
Requested Fixes
packages/ui — Shared Component LibrarySidebar system (new, shared between Planner and ChatBot)
Architecture OptimizationsChatbot: Single Zustand Store Replaces Triple Hook + Event BusBefore: Three independent After: A single
Result: One API call on mount (was two), no race conditions, no Rules of Hooks violation. Planner Sidebar: Deduplication + Mobile ParityTwo sidebar files ( Extracted shared pieces:
Renamed for clarity:
Mobile brought to parity with desktop:
react-router-dom: Moved to peerDependencies
CSS: Tailwind Preset Extracted into packages/uiBefore: After: Design tokens are defined once in PlannerNavbar: Prop Drilling EliminatedBefore: After: |
joalen
left a comment
joalen
left a comment
There was a problem hiding this comment.
Second pass, check it out:
- mobile planner sidebar now shows different categories than before
hasCompletionused to differ between mobile and desktop, where mobile ignored suggested/prereq_blocked when deciding whether to show an AND/OR category, desktop didn't.
This unification means mobile will now render categories that were previously filtered out (any category whose only "completion" is having suggested or prereq-blocked courses, with no actual progress). That's probably the right behavior, but it's a visible UI change on mobile that isn't mentioned in the PR description
Can you check if this was tested on mobile and is intentional, not just an artifact of the merge?
- These
^ feels a bit awkward, could we match it to how it was on the original chatbot site? Basically the three dots (screenshot cut off but the idea there) are needed for each of the convos in the list + the adjustable sidebar here.
Also it looks like the sidebar height is variable rather than static from before so maybe we can keep that behavior or this might happen in which it clips to the bottom
You prob might need to merge from dev into the feat-admin site branch to propagate some of those new UI elements that planner has (like this):
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| startNewChat(); | ||
| onClose?.(); | ||
| }} |
There was a problem hiding this comment.
This calls startNewChat() directly rather than what ChatBot.tsx's handleStartNewChat uses. Desktop saves the current in-progress conversation to the store before clearing activeConversationId so that's good, but the mobile version doesn't get this.
If a user sends a message and taps "new chat" from the mobile drawer before the response lands, that exchange can get dropped.
Can we route this through the same save-then-clear logic, or lift handleStartNewChat somewhere both desktop and mobile can call and retain the same behavior?
There was a problem hiding this comment.
To help a bit, check out the logic in this code definition: handleStartNewChat
| onClose?: () => void; | ||
| layout?: "mobile" | "template"; |
There was a problem hiding this comment.
We can prob add in a property that allows for piping messages in (essentially the same => void) prop for the onClose? but for starting a new chat
| className="w-full flex transition-all duration-100 items-center justify-center space-x-2 py-2 px-6 rounded-3xl bg-accent text-textdark hover:text-gray-700" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| startNewChat(); |
There was a problem hiding this comment.
that new property for the ChatSidebarContentProps will help you out in fixing this issue initially for my startNewChat() comment
| const { | ||
| conversations, | ||
| conversation_id, | ||
| activeConversationId, | ||
| error, | ||
| loading, | ||
| setConversations, | ||
| setActiveConversationId, | ||
| startNewChat, | ||
| deleteConversation, | ||
| renameConversation, | ||
| setConversationId, | ||
| initialLoad | ||
| } = useChatbot(); | ||
| } = useChatbotStore(); |
There was a problem hiding this comment.
Nothing calls that function: initialLoad(user), maybe we can bring back that useEffect?
| renderExpandedContent={ | ||
| <div | ||
| ref={drop} | ||
| className={`${isOver ? 'bg-gray-100' : ''}`} | ||
| style={{ scrollbarWidth: 'none', msOverflowStyle: 'none' }} | ||
| > | ||
| <PlannerDiscoveryBanner onOpenDiscovery={onOpenDiscovery} /> | ||
|
|
||
| {stagedCourses.length > 0 && ( | ||
| <div className="mb-4"> | ||
| <div className="flex items-center justify-between px-1 mb-2"> | ||
| <div className="text-[10px] font-semibold text-purple-500 uppercase tracking-wide flex items-center gap-1.5"> | ||
| <span className="w-2 h-2 rounded-full bg-purple-400 inline-block" /> | ||
| Staged · {stagedCourses.length} course{stagedCourses.length !== 1 ? 's' : ''} | ||
| </div> | ||
| <button | ||
| onClick={() => !allStagedPlaced && usePlannerStore.getState().clearStagedCourses()} | ||
| className={`text-[10px] transition-colors ${allStagedPlaced ? 'text-gray-300 cursor-not-allowed pointer-events-none' : 'text-gray-400 hover:text-red-400 cursor-pointer'}`} | ||
| > | ||
| Clear all | ||
| </button> | ||
| </div> | ||
| <CoursesCarousel | ||
| courses={stagedCourses} | ||
| type="discovered" | ||
| onRemove={removeStagedCourse} | ||
| coursebookData={coursebookData} | ||
| gradesData={gradesData} | ||
| coursebookSemester={coursebookSemester} | ||
| availableSemesters={[]} | ||
| placedSuggestedCourses={placedSuggestedCourses} | ||
| /> | ||
| </div> | ||
| )} | ||
|
|
||
| <h2 className="text-xl font-bold text-gray-900 mb-4">Degree Requirements</h2> | ||
|
|
||
| <div className="space-y-3 pb-24"> | ||
| {requirements.map((req, reqIdx) => { | ||
| const reqCompletion = semesters ? getCompletionForCategory(req, semesters) : { completed: req.progress, total: req.total, isCreditBased: true }; | ||
| const reqCreditsBreakdown = reqCompletion.isCreditBased && semesters ? getCreditsBreakdownRecursive(req, semesters) : undefined; | ||
| return ( | ||
| <RequirementCategory | ||
| key={reqIdx} | ||
| title={req.degree} | ||
| completed={reqCompletion.completed} | ||
| total={reqCompletion.total} | ||
| isExpanded={autoExpandedCategories[reqIdx]} | ||
| onToggle={() => setAutoExpandedCategories((prev) => ({ ...prev, [reqIdx]: !prev[reqIdx] }))} | ||
| hasSubcategories={req.categories?.length > 0} | ||
| isFirstCategory={reqIdx === 0} | ||
| creditsBreakdown={reqCreditsBreakdown} | ||
| footnote={(req as any).footnote} | ||
| rules={(req as any).rules} | ||
| > | ||
| {req.categories?.length > 0 ? ( | ||
| renderCategories(req.categories, reqIdx) | ||
| ) : ( | ||
| <div className="text-sm text-gray-500">No categories available</div> | ||
| )} | ||
| </RequirementCategory> | ||
| ); | ||
| })} | ||
| </div> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
The useDrop ref no longer covers the header area (the "Edit plans" button + collapse toggle row, now rendered by SidebarTemplate itself outside renderExpandedContent). If a course is dropped near the top of the sidebar it may no longer register.
See if we can do a quick manual drag-and-drop test near the top edge before we merge it to our branch so that our behavior doesn't regress from original.
| return ( | ||
| <button | ||
| aria-label={label} | ||
| className={className ?? "flex transition-all duration-100 items-center space-x-2 py-2 px-8 rounded-3xl bg-accent text-textdark text-base hover:text-gray-700"} |
There was a problem hiding this comment.
used to be:
className="flex transition-all duration-100 items-center space-x-2 py-2 px-6 rounded-3xl bg-accent text-textdark hover:text-gray-700"
to which the new one there can introduce a visual regression, check that out
| primaryAction={{ | ||
| label: "Start new chat", | ||
| icon: <MessageCirclePlusIcon size={24} aria-hidden="true" />, | ||
| onClick: onStartNewChat, | ||
| dataTour: "new-chat-expanded", | ||
| }} |
There was a problem hiding this comment.
The shared default (px-8) matches the old planner "Edit plans" button, not the old chat "Start new chat" button (px-6). Since ChatSidebarShell doesn't pass a className override, the chat sidebar's new-chat button picked up slightly wider horizontal padding than before. Minor, but worth a visual diff check to see if this is needed.
| <div className="w-12 h-12 flex items-center justify-center -translate-y-4"> | ||
| {footer ?? <PanelLeftDashed size={24} className="stroke-[#bbbbbb] group-hover/sidebar:stroke-[#dddddd] transition-colors duration-150" />} | ||
| </div> |
There was a problem hiding this comment.
group-hover/sidebar:stroke-[#dddddd] has no matching group/sidebar ancestor in either consumer, so this hover effect is effectively a no-op.
Might have been a preexisting issue carried over from the old code, but since it's now baked into a shared package component, worth either adding group/sidebar to SidebarTemplate's root div or dropping the dead class.
There was a problem hiding this comment.
Ok so if I read the guide right, it tells future devs that we need to manually copy an (incomplete) list of 8 color tokens, but from what i can see, main/tailwind.config.js just does presets: [uiPreset] and gets everything for free: border, secondary, textsecondary, buttondisabled, which the guide's list omits entirely and which shared components (e.g. SidebarTemplate's border border-border) rely on.
Can you update the guide to recommend presets: [uiPreset] instead of manual token duplication? As written, a team following this guide would skip missing borders/secondary colors.
| user={user} | ||
| logout={logout} | ||
| profilePicture={profilePicture} | ||
| triggerClassName={!profilePicture ? "bg-secondary p-2 rounded-full" : "p-2 rounded-full"} |
There was a problem hiding this comment.
Good one! Apparently the old code never applied that bg-secondary mark in which it was because someone forgot to backtick that class.
2 participants
Summary
@sage/ui package is now the shared source of truth for navigation atoms/shell + reusable primitives.
Documentation
Adoption guidelines documented in
packages/ui/ADOPTION_GUIDE.mdNext steps:
Potential CI validation workflow for future site adoption
Closes #190