From 30bbd12f9606c2e99523cb9ece465041cb37e916 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Wed, 16 Mar 2022 18:05:01 -0400 Subject: [PATCH] Update default sortable keyboard coordinate getter --- .changeset/sortable-keyboard-coordinates.md | 12 +++++ cypress.json | 3 +- cypress/integration/sortable_spec.ts | 2 +- cypress/support/commands.ts | 6 ++- .../keyboard/sortableKeyboardCoordinates.ts | 17 +++--- .../Sortable/5-Virtualized.story.tsx | 2 + .../Sortable/MultipleContainers.tsx | 4 +- stories/2 - Presets/Sortable/Sortable.tsx | 2 + .../multipleContainersKeyboardCoordinates.ts} | 16 ++++-- .../Advanced/Pages/Page.module.css | 9 ++-- .../Games/PlayingCards/PlayingCards.story.tsx | 3 -- .../3 - Examples/Tree/keyboardCoordinates.ts | 53 +++++++++---------- 12 files changed, 77 insertions(+), 52 deletions(-) create mode 100644 .changeset/sortable-keyboard-coordinates.md rename stories/{3 - Examples/Games/PlayingCards/keyboardCoordinates.ts => 2 - Presets/Sortable/multipleContainersKeyboardCoordinates.ts} (81%) diff --git a/.changeset/sortable-keyboard-coordinates.md b/.changeset/sortable-keyboard-coordinates.md new file mode 100644 index 00000000..f7204db2 --- /dev/null +++ b/.changeset/sortable-keyboard-coordinates.md @@ -0,0 +1,12 @@ +--- +'@dnd-kit/sortable': major +--- + +The default `sortableKeyboardCoordinates` function that is exported from the `@dnd-kit/sortable` package has been updated to better handle cases where the collision rectangle is overlapping droppable rectangles. For example, for `down` arrow key, the default function had logic that would only consider collisions against droppables that were below the `bottom` edge of the collision rect. This was problematic when the collision rect was overlapping droppable rects, because it meant that it's bottom edge was below the top edge of the droppable, and that resulted in that droppable being skipped. + +```diff +- collisionRect.bottom > droppableRect.top ++ collisionRect.top > droppableRect.top +``` + +This change should be backwards compatible for most consumers, but may introduce regressions in some use-cases, especially for consumers that may have copied the multiple containers examples. There is now a custom sortable keyboard coordinate getter [optimized for multiple containers that you can refer to](https://github.com/clauderic/dnd-kit/tree/master/stories/2%20-%20Presets/Sortable/multipleContainersKeyboardCoordinates.ts). diff --git a/cypress.json b/cypress.json index b60db64f..6cc83442 100644 --- a/cypress.json +++ b/cypress.json @@ -3,5 +3,6 @@ "supportFile": "cypress/support/index.ts", "projectId": "zkn9qu", "baseUrl": "http://localhost:6006/", - "viewportHeight": 1000 + "viewportHeight": 1000, + "scrollBehavior": false } diff --git a/cypress/integration/sortable_spec.ts b/cypress/integration/sortable_spec.ts index ecf2d055..9e9f398d 100644 --- a/cypress/integration/sortable_spec.ts +++ b/cypress/integration/sortable_spec.ts @@ -518,7 +518,7 @@ describe('Sortable Virtualized List', () => { }); describe('Stress test', () => { - it.skip('Multiple actions in both directions', () => { + it('Multiple actions in both directions', () => { const initialIndex = 10; const id = getIdForIndex(initialIndex); const delta = 10; diff --git a/cypress/support/commands.ts b/cypress/support/commands.ts index 6b005af7..7036522d 100644 --- a/cypress/support/commands.ts +++ b/cypress/support/commands.ts @@ -148,7 +148,11 @@ Cypress.Commands.add( force: true, }) .wait(150) - .type(Keys.Space, {scrollBehavior: false, log: false, force: true}); + .type(Keys.Space, { + force: true, + scrollBehavior: false, + }) + .wait(250); } ); diff --git a/packages/sortable/src/sensors/keyboard/sortableKeyboardCoordinates.ts b/packages/sortable/src/sensors/keyboard/sortableKeyboardCoordinates.ts index 6450af12..4d0193cb 100644 --- a/packages/sortable/src/sensors/keyboard/sortableKeyboardCoordinates.ts +++ b/packages/sortable/src/sensors/keyboard/sortableKeyboardCoordinates.ts @@ -19,9 +19,10 @@ export const sortableKeyboardCoordinates: KeyboardCoordinateGetter = ( { context: { active, + collisionRect, droppableRects, droppableContainers, - collisionRect, + over, scrollableAncestors, }, } @@ -48,22 +49,22 @@ export const sortableKeyboardCoordinates: KeyboardCoordinateGetter = ( switch (event.code) { case KeyboardCode.Down: - if (collisionRect.top + collisionRect.height <= rect.top) { + if (collisionRect.top < rect.top) { filteredContainers.push(entry); } break; case KeyboardCode.Up: - if (collisionRect.top >= rect.top + rect.height) { + if (collisionRect.top > rect.top) { filteredContainers.push(entry); } break; case KeyboardCode.Left: - if (collisionRect.left >= rect.left + rect.width) { + if (collisionRect.left > rect.left) { filteredContainers.push(entry); } break; case KeyboardCode.Right: - if (collisionRect.left + collisionRect.width <= rect.left) { + if (collisionRect.left < rect.left) { filteredContainers.push(entry); } break; @@ -77,7 +78,11 @@ export const sortableKeyboardCoordinates: KeyboardCoordinateGetter = ( droppableContainers: filteredContainers, pointerCoordinates: null, }); - const closestId = getFirstCollision(collisions, 'id'); + let closestId = getFirstCollision(collisions, 'id'); + + if (closestId === over?.id && collisions.length > 1) { + closestId = collisions[1].id; + } if (closestId != null) { const newDroppable = droppableContainers.get(closestId); diff --git a/stories/2 - Presets/Sortable/5-Virtualized.story.tsx b/stories/2 - Presets/Sortable/5-Virtualized.story.tsx index b32299d3..8676188a 100644 --- a/stories/2 - Presets/Sortable/5-Virtualized.story.tsx +++ b/stories/2 - Presets/Sortable/5-Virtualized.story.tsx @@ -43,6 +43,8 @@ function Sortable({ const sensors = useSensors( useSensor(PointerSensor), useSensor(KeyboardSensor, { + // Disable smooth scrolling in Cypress automated tests + scrollBehavior: 'Cypress' in window ? 'auto' : undefined, coordinateGetter: sortableKeyboardCoordinates, }) ); diff --git a/stories/2 - Presets/Sortable/MultipleContainers.tsx b/stories/2 - Presets/Sortable/MultipleContainers.tsx index 15faa164..0b543c67 100644 --- a/stories/2 - Presets/Sortable/MultipleContainers.tsx +++ b/stories/2 - Presets/Sortable/MultipleContainers.tsx @@ -28,12 +28,12 @@ import { useSortable, arrayMove, defaultAnimateLayoutChanges, - sortableKeyboardCoordinates, verticalListSortingStrategy, SortingStrategy, horizontalListSortingStrategy, } from '@dnd-kit/sortable'; import {CSS} from '@dnd-kit/utilities'; +import {coordinateGetter as multipleContainersCoordinateGetter} from './multipleContainersKeyboardCoordinates'; import {Item, Container, ContainerProps} from '../../components'; @@ -151,7 +151,7 @@ export function MultipleContainers({ handle = false, items: initialItems, containerStyle, - coordinateGetter = sortableKeyboardCoordinates, + coordinateGetter = multipleContainersCoordinateGetter, getItemStyles = () => ({}), wrapperStyle = () => ({}), minimal = false, diff --git a/stories/2 - Presets/Sortable/Sortable.tsx b/stories/2 - Presets/Sortable/Sortable.tsx index 353ef790..9b43a962 100644 --- a/stories/2 - Presets/Sortable/Sortable.tsx +++ b/stories/2 - Presets/Sortable/Sortable.tsx @@ -122,6 +122,8 @@ export function Sortable({ activationConstraint, }), useSensor(KeyboardSensor, { + // Disable smooth scrolling in Cypress automated tests + scrollBehavior: 'Cypress' in window ? 'auto' : undefined, coordinateGetter, }) ); diff --git a/stories/3 - Examples/Games/PlayingCards/keyboardCoordinates.ts b/stories/2 - Presets/Sortable/multipleContainersKeyboardCoordinates.ts similarity index 81% rename from stories/3 - Examples/Games/PlayingCards/keyboardCoordinates.ts rename to stories/2 - Presets/Sortable/multipleContainersKeyboardCoordinates.ts index 188a84c2..0967be76 100644 --- a/stories/3 - Examples/Games/PlayingCards/keyboardCoordinates.ts +++ b/stories/2 - Presets/Sortable/multipleContainersKeyboardCoordinates.ts @@ -15,7 +15,7 @@ const directions: string[] = [ export const coordinateGetter: KeyboardCoordinateGetter = ( event, - {context: {active, droppableContainers, collisionRect}} + {context: {active, droppableRects, droppableContainers, collisionRect}} ) => { if (directions.includes(event.code)) { event.preventDefault(); @@ -31,7 +31,7 @@ export const coordinateGetter: KeyboardCoordinateGetter = ( return; } - const rect = entry?.rect.current; + const rect = droppableRects.get(entry.id); if (!rect) { return; @@ -43,7 +43,9 @@ export const coordinateGetter: KeyboardCoordinateGetter = ( const {type, children} = data; if (type === 'container' && children?.length > 0) { - return; + if (active.data.current?.type !== 'container') { + return; + } } } @@ -74,6 +76,7 @@ export const coordinateGetter: KeyboardCoordinateGetter = ( const collisions = closestCorners({ active, collisionRect: collisionRect, + droppableRects, droppableContainers: filteredContainers, pointerCoordinates: null, }); @@ -85,6 +88,13 @@ export const coordinateGetter: KeyboardCoordinateGetter = ( const newRect = newDroppable?.rect.current; if (newNode && newRect) { + if (newDroppable.data.current?.type === 'container') { + return { + x: newRect.left + (newRect.width - collisionRect.width) / 2, + y: newRect.top + (newRect.height - collisionRect.height) / 2, + }; + } + return { x: newRect.left, y: newRect.top, diff --git a/stories/3 - Examples/Advanced/Pages/Page.module.css b/stories/3 - Examples/Advanced/Pages/Page.module.css index 86fdd260..051eee5d 100644 --- a/stories/3 - Examples/Advanced/Pages/Page.module.css +++ b/stories/3 - Examples/Advanced/Pages/Page.module.css @@ -16,11 +16,8 @@ } &.clone { - margin-top: 10px; - margin-left: 10px; - .Page { - transform: scale(1.025); + transform: translate3d(10px, 10px, 0) scale(1.025); animation: pop 150ms cubic-bezier(0.18, 0.67, 0.6, 1.22); box-shadow: 0 0 0 1px rgba(63, 63, 68, 0.05), 0 1px 6px 0 rgba(34, 33, 81, 0.3); @@ -207,9 +204,9 @@ @keyframes pop { 0% { - transform: translate3d(-10px, -10px, 0) scale(1); + transform: translate3d(0px, 0px, 0) scale(1); } 100% { - transform: translate3d(0px, 0px, 0) scale(1.025); + transform: translate3d(10px, 10px, 0) scale(1.025); } } diff --git a/stories/3 - Examples/Games/PlayingCards/PlayingCards.story.tsx b/stories/3 - Examples/Games/PlayingCards/PlayingCards.story.tsx index 91433ba6..8f54c14e 100644 --- a/stories/3 - Examples/Games/PlayingCards/PlayingCards.story.tsx +++ b/stories/3 - Examples/Games/PlayingCards/PlayingCards.story.tsx @@ -4,7 +4,6 @@ import { rectSortingStrategy, } from '@dnd-kit/sortable'; -import {coordinateGetter} from './keyboardCoordinates'; import {Sortable} from '../../../2 - Presets/Sortable/Sortable'; import {MultipleContainers} from '../../../2 - Presets/Sortable/MultipleContainers'; import {PlayingCard, getDeckOfCards} from './PlayingCard'; @@ -50,7 +49,6 @@ export const SingleDeck = () => { zIndex: isDragging ? deck.length - overIndex : deck.length - index, opacity: isDragging && !isDragOverlay ? 0.3 : undefined, })} - coordinateGetter={coordinateGetter} /> ); @@ -152,7 +150,6 @@ export const MultipleDecks = () => { : deck.length - index, }; }} - coordinateGetter={coordinateGetter} minimal /> diff --git a/stories/3 - Examples/Tree/keyboardCoordinates.ts b/stories/3 - Examples/Tree/keyboardCoordinates.ts index 4638228f..18d53ea6 100644 --- a/stories/3 - Examples/Tree/keyboardCoordinates.ts +++ b/stories/3 - Examples/Tree/keyboardCoordinates.ts @@ -26,7 +26,7 @@ export const sortableTreeKeyboardCoordinates: ( event, { currentCoordinates, - context: {active, over, collisionRect, droppableContainers}, + context: {active, over, collisionRect, droppableRects, droppableContainers}, } ) => { if (directions.includes(event.code)) { @@ -73,41 +73,36 @@ export const sortableTreeKeyboardCoordinates: ( const containers: DroppableContainer[] = []; - const overRect = over?.id - ? droppableContainers.get(over.id)?.rect.current - : undefined; - - if (overRect) { - droppableContainers.forEach((container) => { - if (container?.disabled) { - return; - } + droppableContainers.forEach((container) => { + if (container?.disabled || container.id === over?.id) { + return; + } - const rect = container?.rect.current; + const rect = container?.rect.current; - if (!rect) { - return; - } + if (!rect) { + return; + } - switch (event.code) { - case KeyboardCode.Down: - if (overRect.top < rect.top) { - containers.push(container); - } - break; - case KeyboardCode.Up: - if (overRect.top > rect.top) { - containers.push(container); - } - break; - } - }); - } + switch (event.code) { + case KeyboardCode.Down: + if (collisionRect.top < rect.top) { + containers.push(container); + } + break; + case KeyboardCode.Up: + if (collisionRect.top > rect.top) { + containers.push(container); + } + break; + } + }); const collisions = closestCorners({ active, - collisionRect: collisionRect, + collisionRect, pointerCoordinates: null, + droppableRects, droppableContainers: containers, }); const closestId = getFirstCollision(collisions, 'id');