Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds drag-and-drop functionality to the home page of the Ograf DevTool, allowing users to drop folders directly onto the page instead of having to click a button to select them. The implementation uses the File System Access API's getAsFileSystemHandle method to handle dropped folders.
Changes:
- Added drag-and-drop event handlers (dragOver, dragEnter, dragLeave, drop) to the InitialView component
- Implemented visual feedback with a blue dashed outline when dragging over the drop zone
- Refactored folder selection logic into a reusable handleFolderSelect callback
- Added error handling and user-friendly error messages for various failure scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| client/src/views/InitialView.jsx | Added drag-and-drop state management, event handlers, and refactored folder selection logic into a shared callback |
| client/src/scss/styles.scss | Added visual feedback styling for drag state and pointer-events configuration to allow drag events through content |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handleDragLeave = React.useCallback((e) => { | ||
| e.preventDefault() | ||
| e.stopPropagation() | ||
| setIsDragging(false) | ||
| }, []) |
There was a problem hiding this comment.
The handleDragLeave implementation will trigger when dragging over child elements within the drop zone, causing the dragging visual feedback to flicker. This happens because dragLeave fires when entering child elements like the heading, paragraphs, or button.
To fix this, you need to track if the drag actually left the container by checking the relatedTarget or by using a reference counter pattern. One common approach is to only reset isDragging to false when the related target is not a descendant of the drop zone container.
| }) | ||
| } catch (err) { | ||
| console.error(err) | ||
| setError(err.message) |
There was a problem hiding this comment.
The error message access pattern 'err.message' may not be safe for all error types. If err is null, undefined, or doesn't have a message property, this could fail. While there's a fallback in the handleDrop function (line 73), this catch block doesn't have one.
Consider using a more defensive pattern like 'err?.message || String(err)' to ensure an error message is always available.
| setError(err.message) | |
| setError(err?.message || String(err)) |
| .catch((err) => { | ||
| console.error(err) | ||
| setError(err.message) |
There was a problem hiding this comment.
The error message access pattern 'err.message' may not be safe for all error types. If err is null, undefined, or doesn't have a message property, this could fail.
Consider using a more defensive pattern like 'err?.message || String(err)' to ensure an error message is always available, similar to the fallback used in the handleDrop function at line 73.
| onDrop={handleDrop} | ||
| > |
There was a problem hiding this comment.
The drag and drop zone lacks ARIA attributes to make it accessible to screen readers and assistive technologies. Users who rely on keyboard navigation or screen readers won't be aware that this area accepts folder drops.
Consider adding appropriate ARIA attributes such as 'role="region"' and 'aria-label' to describe the drop zone functionality. You might also want to add 'aria-live' to announce when the dragging state changes.
| onDrop={handleDrop} | |
| > | |
| onDrop={handleDrop} | |
| role="region" | |
| aria-label="Graphics folder drop zone. Drag and drop a folder here or use the button below." | |
| tabIndex={0} | |
| > | |
| <div | |
| aria-live="polite" | |
| style={{ | |
| position: 'absolute', | |
| width: '1px', | |
| height: '1px', | |
| margin: '-1px', | |
| border: 0, | |
| padding: 0, | |
| clip: 'rect(0 0 0 0)', | |
| overflow: 'hidden', | |
| whiteSpace: 'nowrap', | |
| }} | |
| > | |
| {isDragging | |
| ? 'Folder detected over drop zone. Release to load graphics.' | |
| : 'Drag and drop a graphics folder onto this region to load graphics.'} | |
| </div> |
| pointer-events: none; // Allow drag events to bubble through | ||
| } | ||
| a { | ||
| color: #bbf; | ||
| text-decoration: underline; | ||
| pointer-events: all; // Re-enable pointer events for links | ||
| } | ||
| button { | ||
| pointer-events: all; // Re-enable pointer events for buttons |
There was a problem hiding this comment.
Setting 'pointer-events: none' on the content and then re-enabling it for specific elements is a fragile pattern. If any new interactive elements (inputs, selects, other buttons, etc.) are added to the initial-hero-content in the future, they will be non-interactive unless explicitly given 'pointer-events: all'.
Consider a more maintainable approach, such as handling drag events at the container level and using event.target checks, or using a more specific selector for elements that should not block drag events.
| pointer-events: none; // Allow drag events to bubble through | |
| } | |
| a { | |
| color: #bbf; | |
| text-decoration: underline; | |
| pointer-events: all; // Re-enable pointer events for links | |
| } | |
| button { | |
| pointer-events: all; // Re-enable pointer events for buttons | |
| } | |
| a { | |
| color: #bbf; | |
| text-decoration: underline; | |
| } | |
| button { |
1 participant
Instead of having to select a folder.