fix/416 cannot drag control #417
Conversation
flex controled height is not visible to sortablejs
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #416 where controls cannot be dragged to the stage. The fix involves refactoring the SortableJS configuration for controls, adding forceFallback: true across drag-enabled components, implementing overflow hiding during drag to prevent scrollbar flashing, and adding comprehensive Playwright tests to verify the drag-and-drop functionality.
Key Changes
- Refactored SortableJS drag event handlers: moved ghost preview logic from
onStarttoonClone, added overflow management inonStart/onEndto prevent scrollbar flashing - Added
forceFallback: trueto Sortable configurations in controls, rows, and columns for consistent drag behavior - Added
min-height: 100%to stage children container to ensure adequate drop zone height - Created comprehensive Playwright test suite covering click-to-add, drag-to-add, multiple controls, layout stability, and overflow behavior
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 |
|---|---|
| tests/drag-control-to-stage.spec.js | New Playwright test suite with 6 tests covering control addition via click, drag operations, layout stability, and overflow behavior during drag |
| src/lib/sass/components/_stage.scss | Added min-height: 100% to stage children container to ensure proper drop zone height |
| src/lib/js/components/rows/row.js | Added forceFallback: true to Sortable configuration for consistent drag behavior |
| src/lib/js/components/controls/index.js | Major refactoring: moved ghost preview to onClone, added overflow hiding in onStart/onEnd, added revertClone: true, changed window to globalThis for localStorage access |
| src/lib/js/components/columns/column.js | Added forceFallback: true to Sortable configuration for consistent drag behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import('../fields/field.js').then(({ default: Field }) => { | ||
| clone.innerHTML = '' | ||
| clone.appendChild(new Field(controlData).preview) | ||
| }) |
There was a problem hiding this comment.
The dynamic import in onClone lacks error handling. If the import fails, the promise rejection will be unhandled, which could cause issues in some environments or make debugging harder.
Consider adding a .catch() handler:
import('../fields/field.js')
.then(({ default: Field }) => {
clone.innerHTML = ''
clone.appendChild(new Field(controlData).preview)
})
.catch(err => console.error('Failed to load Field for ghost preview:', err))| }) | |
| }).catch(err => console.error('Failed to load Field for ghost preview:', err)) |
| const overflowAfterDrag = await page.evaluate(() => document.documentElement.style.overflow) | ||
| expect(overflowAfterDrag).not.toBe('hidden') |
There was a problem hiding this comment.
The assertion expect(overflowAfterDrag).not.toBe('hidden') could fail if the original overflow style was an empty string (which is the default). After restoration, it should return to its original value, but this test will pass even if overflow is incorrectly set to 'auto', 'scroll', or any other value.
Consider using a more specific assertion:
expect(overflowAfterDrag).toBe('')or verify it matches the initial overflow value captured before the test.
There was a problem hiding this comment.
its possible a developer or other code has set a value. we want to prevent breaking any functionality so reverting to whatever it was prior is a safer method.
|
🎉 This PR is included in version 4.2.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
resolves DEMO not working in firefox #416