Catalog to planner dnd - #113
Conversation
Yeah, it can do it now but it looks a little awkward. Will fix in next commit.
Now, when a course is dragged from the course list, the blue copy is spawned and be dragged accordingly.
There was a problem hiding this comment.
Pull request overview
Adds catalog-to-planner drag-and-drop support so users can drag a course card from the Catalog and drop it into a Planner semester, creating a new UserCourse entry and showing a compact planner-style drag ghost.
Changes:
- Made catalog course cards draggable with DnD metadata (
type: "catalog-course") and visual feedback while dragging. - Added catalog-drop handling to the workspace DnD provider, including a
DragOverlayghost and UUID assignment for new planner entries. - Expanded planner semester drop acceptance and improved button accessibility attributes across several components.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/features/toolbox/GarbageBin.tsx | Adds button semantics (type, aria-label) to the garbage dropzone. |
| src/features/planner/components/semester/SemesterBlock.tsx | Allows semesters to accept "catalog-course" drops; adds button semantics/labels. |
| src/features/dnd/dnd.tsx | Implements catalog-to-planner drop logic and a planner-style DragOverlay ghost. |
| src/features/catalog/components/CatalogCourse.tsx | Wires catalog cards as drag sources via useDraggable and dims while dragging. |
| src/components/course/Course.tsx | Adds button semantics/labels to planner course drag handle and options trigger. |
Comments suppressed due to low confidence (2)
src/features/dnd/dnd.tsx:191
handleDragEndresets planner/toolbox state on cancel/no-target using snapshots. For catalog drags, state is not mutated duringonDragOver, so this reset can unnecessarily revert unrelated in-flight workspace changes that happened while dragging. Consider skipping the snapshot reset forsource.data.type === "catalog-course", or only snapshot/reset for drag types that actually perform optimistic updates during drag.
const { source, target } = event.operation;
if (event.canceled || !target) {
resetPlanner(snapshotPlanner.current);
resetToolbox(snapshotToolbox.current);
return;
src/features/toolbox/GarbageBin.tsx:56
- When
showBinis false, the button is visually hidden viaopacity-0andpointer-events-none, but it remains in the tab order and discoverable to screen readers. Consider also settingdisabled/aria-hidden/tabIndex={-1}when hidden, or conditionally rendering the button only while a draggable course is active, to avoid focusing an invisible control.
<button
ref={ref}
type="button"
aria-label="Delete course"
className={cn(
"w-fit transition-opacity duration-200",
showBin ? "opacity-100" : "opacity-0 pointer-events-none",
)}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Skip snapshots in handleDragStart for catalog drags since onDragOver is a no-op for catalog-course and no rollback is needed on drop - Stop pointerdown propagation on the Add button wrapper to prevent the drag sensor from initiating a drag on what should be a click
ramonechen
left a comment
ramonechen
left a comment
There was a problem hiding this comment.
I've identified a few issues in this change:
-
Browser local storage seems to get nuked every time a course is dragged directly from the catalog to the planner (i.e. refreshing the page after adding a course directly from the catalog will reset the entire planner). Courses still seem to be saved properly when adding to the planner via the toolbox.
-
I don't seem to be able to drag courses from the catalog to the toolbox. I think it's natural for the user to try this, so I would like to see this addressed if it's not too complicated.
-
The credit count in the credit selector is missing for courses that are dragged directly from the catalog (image below). This is not an issue, however, for courses dragged from the toolbox.
- The cloned, translucent course preview does not appear when dragging courses directly from the catalog into the planner. Without this preview, it's somewhat difficult to determine where the course will be dropped exactly when I release left-click. This preview still appears normally for courses being dragged from the toolbox.
Each CatalogCourse now generates a stable UUID on mount via useRef, preventing ID collisions when multiple instances of the same course are visible. Adds a MdDragIndicator icon as a visual affordance.
…redits - Take planner/toolbox snapshots at drag start for all drag types, including catalog drags, so cancellation correctly restores state - Remove early return for catalog-course in handleDragOver; catalog courses now live-insert into the hovered semester during drag, matching the toolbox-to-planner pattern and restoring the ghost preview and course reordering feedback - Add missing credits field (credit_max) when constructing UserCourse from an APICourse during a catalog drag
Catalog card dragging is disabled on touch devices since there is nowhere to drop. The catalog results container switches to overflow-hidden while a catalog drag is active so the AutoScroller cannot scroll the list in a direction that has no valid drop targets.
The catalog card's draggable ID (stable per mount) was being used as the planner course ID. After the first drop, courseLocationMap found that ID in the planner and treated every subsequent drag from the same card as a move rather than a new insertion. Fix: generate a fresh UUID in handleDragStart for each catalog drag session and use that as the planner course ID instead of sourceId.
- Move useIsDesktop import before @/lib/types to satisfy import-x/order - Remove addCourseToSemester from handleDragEnd deps array; it is no longer called there after the catalog logic moved to handleDragOver
| // --- CATALOG TO PLANNER --- | ||
| if (sourceType === "catalog-course") { | ||
| const apiCourse = source.data?.course as APICourse | undefined; | ||
| if (!apiCourse) return; | ||
|
|
||
| const targetSemesterId = | ||
| targetType === "semester" | ||
| ? target.data?.semesterId || targetId | ||
| : courseLocationMap.get(targetId)?.semesterId; | ||
|
|
||
| if (!targetSemesterId) return; | ||
|
|
||
| let catalogTargetIndex: number | undefined; | ||
| if (isSortable(target)) { | ||
| catalogTargetIndex = target.index; | ||
| } else { | ||
| const targetSemester = | ||
| plannerCourses[ |
There was a problem hiding this comment.
I would reference the behavior of the course preview when dragging a course from the toolbox into the planner. Dragging a course from the catalog into the planner should do the same thing for consistency's sake.
Dragging from the toolbox into a semester shows a preview of the course inside that semester based on the drag position of that dragged course. It's not until the user stops dragging that the course is visually added officially. Right now, it seems like it is just added officially.
There was a problem hiding this comment.
Also, I am not sure where this is coming from but there seems to be some type of shadow on the semester dropzone when hovering a course from the catalog over a semester. This is not present when for courses that originate from the toolbox so it should also be removed.
There was a problem hiding this comment.
The transparent placeholder and the semester shadow are both addressed in commit 91586b7: isCatalogPlaceholder in Course.tsx makes the in-planner placeholder opacity-0 to match toolbox-drag behaviour, and the semester bg-black/5 highlight is suppressed while a catalog drag is active.
There was a problem hiding this comment.
The shadow on the semester dropzone is suppressed during catalog drags via the isDraggingCatalog check in SemesterBlock.tsx, committed in 91586b7.
…uring drag - MdDragIndicator is only rendered on desktop since mobile dragging is disabled - Apply overflow-hidden to the inner results div during catalog drags, not just the outer container, so the AutoScroller cannot scroll the list
… opacity - Extract CatalogDragOverlay into a child component of DragDropProvider that reads useDragOperation directly, eliminating the React state cycle that caused a visible delay before the drag overlay appeared - Expose the in-flight catalog planner course ID via CatalogDragContext so Course.tsx can apply opacity-0 to the inserted placeholder, matching the ghost preview behavior of toolbox-to-planner drags - Suppress the semester dropzone bg-black/5 highlight during catalog drags; the highlight was absent for toolbox drags because those transition to targeting individual planner-course sortables, but catalog drags always kept the semester droppable as the active target
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: liug88 <85533492+liug88@users.noreply.github.com>
- Fix: catalog drags no longer reset the planner on drop; only an explicit Escape cancels and undoes the placed placeholder - Add: catalog courses can now be dragged directly into the Toolbox; hovering over the toolbox removes the planner placeholder for visual clarity, and confirming the drop smart-adds the course to the toolbox (increments count for duplicates) - Also include catalog-course in the Toolbox's accept list Agent-Logs-Url: https://github.com/Project-CARPI/site/sessions/294eba98-1f9d-4cc4-9196-c142d01155a5 Co-authored-by: liug88 <85533492+liug88@users.noreply.github.com>
Merged
All four points addressed in commit
|
Agent-Logs-Url: https://github.com/Project-CARPI/site/sessions/294eba98-1f9d-4cc4-9196-c142d01155a5 Co-authored-by: liug88 <85533492+liug88@users.noreply.github.com>
Hopefully, this should fix the rest of the changes.
…ract
Addresses the review comment about the catalog->planner and
toolbox->planner drag paths diverging visually.
The catalog drag mounts a real <Course /> into planner state during
hover as the in-flight placeholder. Previously that element was styled
with ad-hoc Tailwind (opacity-50, pointer-events-none, and
conditional hover:shadow-lg suppression) to mimic the toolbox ghost.
The toolbox path gets the same look for free from dnd-kit, which
emits this contract on its placeholder clones:
data-dnd-placeholder="clone" inert aria-hidden="true" tabindex="-1"
That contract, combined with the `[data-dnd-placeholder="clone"]
{ opacity: 0.5 !important }` rule already present in index.css and
the `inert` attribute, handles all four concerns uniformly:
0.5 opacity, no pointer events, no hover shadow, no keyboard focus.
This commit makes PlannerCourseView emit the same attributes when
rendering as the catalog placeholder (new `isCatalogPlaceholder`
prop) and drops the Tailwind overrides. The two flows now share the
same rendering contract -- inspect the DOM mid-drag and the two
placeholders are identical in attributes, driven by the same CSS
rule.
The split between useSortable (toolbox) and useDraggable (catalog)
stays intentional -- they describe genuinely different data sources
(stateful list item vs. read-only template) -- but the rendering
layer now converges.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
I don't understand what the purpose of this context is. Catalog dragging should be handle by the global DragDropProvider. Placeholders should also be able to be made visible from this as well. Please reference how it is done with the other drag and drop operations
There was a problem hiding this comment.
Thanks for the insight! Just refactored. I Deleted CatalogDragContext entirely since the global DragDropProvider already publishes the active operation through useDragOperation(), so there's no reason to layer a second context on top. handleDragStart now just stashes a fresh placeholderId on source.data, and every consumer (Course, handleDragOver, handleDragEnd) reads it off the operation the DragDropProvider exposes. Should be the same pattern the rest of the DnD already uses.
I also tried to fix the missing toolbox placeholder: when a catalog drag hovers the toolbox, handleDragOver now inserts a ghost UserCourse there (and reorders it on further hover), mirroring the planner behavior. A shared getCatalogPlaceholderAttrs helper applies data-dnd-placeholder="clone" + inert to both planner and toolbox variants of Course, so the ghost styling is uniform regardless of which container the placeholder is in.
There was a problem hiding this comment.
Similar to the lack of a planner course placeholder, I also just noticed that there is a lack of a toolbox course placeholder when hovering a course that came from the catalog over the toolbox.
This should be the final fix for the over using the DragDropProvider.
Catalog to Planner Drag and Drop
Adds the ability to drag courses directly from the Catalog into a Planner semester block.
Changes
Drag source —
CatalogCourse.tsxuseDraggablewith type"catalog-course"so each catalog card is draggableDrag ghost —
dnd.tsx+DragOverlayDragOverlayAPICourseis captured in state ondragStartand cleared ondragEnd, which feeds static children toDragOverlay— this avoids relying on dnd-kit's reactive signal internals for renderingdropAnimation={null}prevents the ghost from snapping back to the catalog on a successful dropDrop target —
SemesterBlock.tsx"catalog-course"to each semester'sacceptlist so catalog cards can land in semester blocksDrop logic —
dnd.tsxonDragEndonly (notonDragOver) because they create a newUserCourseentry rather than moving an existing one — processing it live on every hover would be prematureAccessibility
type="button"andaria-labelto drag handles, the options button, the add-to-toolbox button, and the garbage bin button