Improved drag flow (#172)

Cleaning up drag lift flow to be more simple and robust. Adding tests for lift flow
atlassian · Nov 16, 2017 · 41101c7 · 41101c7
1 parent e73251d
commit 41101c7
Show file tree

Hide file tree

Showing 5 changed files with 328 additions and 65 deletions.
diff --git a/src/state/action-creators.js b/src/state/action-creators.js
@@ -55,14 +55,6 @@ export const requestDimensions = (type: TypeId): RequestDimensionsAction => ({
   payload: type,
 });
 
-export type BeginLiftAction = {|
-  type: 'BEGIN_LIFT'
-|}
-
-const beginLift = (): BeginLiftAction => ({
-  type: 'BEGIN_LIFT',
-});
-
 export type CompleteLiftAction = {|
   type: 'COMPLETE_LIFT',
   payload: {|
@@ -74,7 +66,7 @@ export type CompleteLiftAction = {|
   |}
 |}
 
-const completeLift = (id: DraggableId,
+export const completeLift = (id: DraggableId,
   type: TypeId,
   client: InitialDragLocation,
   windowScroll: Position,
@@ -233,6 +225,16 @@ export const clean = (): CleanAction => ({
   payload: null,
 });
 
+type PrepareAction = {
+  type: 'PREPARE',
+  payload: null,
+}
+
+export const prepare = (): PrepareAction => ({
+  type: 'PREPARE',
+  payload: null,
+});
+
 export type DropAnimateAction = {
   type: 'DROP_ANIMATE',
   payload: {|
@@ -279,26 +281,21 @@ export const drop = () =>
   (dispatch: Dispatch, getState: () => State): void => {
     const state: State = getState();
 
-    // This can occur if the user ends a drag before
-    // the collecting phase is finished.
-    // This will not trigger a hook as the hook waits
-    // for a DRAGGING phase before firing a onDragStart
+    // dropped before a drag officially started - this is fine
     if (state.phase === 'COLLECTING_DIMENSIONS') {
-      console.error('canceling drag while collecting');
       dispatch(clean());
       return;
     }
 
+    // dropped in another phase except for dragging - this is an error
     if (state.phase !== 'DRAGGING') {
-      console.error('cannot drop if not dragging', state);
-      // We need to wrap this in a setTimeout to avoid a race
-      // condition with `lift`, which includes timeouts
-      setTimeout(() => dispatch(clean()));
+      console.error(`not able to drop in phase: '${state.phase}'`);
+      dispatch(clean());
       return;
     }
 
     if (!state.drag) {
-      console.error('invalid drag state', state);
+      console.error('not able to drop when there is invalid drag state', state);
       dispatch(clean());
       return;
     }
@@ -427,55 +424,55 @@ export type LiftAction = {|
   |}
 |}
 
-// using redux-thunk
 export const lift = (id: DraggableId,
   type: TypeId,
   client: InitialDragLocation,
   windowScroll: Position,
   isScrollAllowed: boolean,
 ) => (dispatch: Dispatch, getState: Function) => {
-  (() => {
-    const state: State = getState();
-    // quickly finish any current animations
-    if (state.phase === 'DROP_ANIMATING') {
-      if (!state.drop || !state.drop.pending) {
-        console.error('cannot flush drop animation if there is no pending');
-        dispatch(clean());
-        return;
-      }
-      dispatch(completeDrop(state.drop.pending.result));
+  // Phase 1: Quickly finish any current drop animations
+  const initial: State = getState();
+
+  if (initial.phase === 'DROP_ANIMATING') {
+    if (!initial.drop || !initial.drop.pending) {
+      console.error('cannot flush drop animation if there is no pending');
+      dispatch(clean());
+    } else {
+      dispatch(completeDrop(initial.drop.pending.result));
     }
-  })();
+  }
 
   // https://github.com/chenglou/react-motion/issues/437
   // need to allow a flush of react-motion
+  dispatch(prepare());
+
   setTimeout(() => {
+    // Phase 2: collect all dimensions
     const state: State = getState();
 
-    if (state.phase !== 'IDLE' && state.phase !== 'DRAG_COMPLETE') {
-      dispatch(clean());
+    // drag cancelled before timeout finished
+    if (state.phase !== 'PREPARING') {
+      return;
     }
 
-    dispatch(beginLift());
     dispatch(requestDimensions(type));
 
-    // Dimensions will be requested synchronously
-    // after they are done - lift.
-    // Could improve this by explicitly waiting until all dimensions are published.
-    // Could also allow a lift to occur before all the dimensions are published
+    // Need to allow an opportunity for the dimensions to be requested.
     setTimeout(() => {
+      // Phase 3: dimensions are collected: start a lift
       const newState: State = getState();
 
       // drag was already cancelled before dimensions all collected
       if (newState.phase !== 'COLLECTING_DIMENSIONS') {
         return;
       }
+
       dispatch(completeLift(id, type, client, windowScroll, isScrollAllowed));
     });
   });
 };
 
-export type Action = BeginLiftAction |
+export type Action =
   CompleteLiftAction |
   RequestDimensionsAction |
   PublishDraggableDimensionAction |
@@ -487,4 +484,5 @@ export type Action = BeginLiftAction |
   CrossAxisMoveBackwardAction |
   DropAnimateAction |
   DropCompleteAction |
+  PrepareAction |
   CleanAction;
diff --git a/src/state/reducer.js b/src/state/reducer.js
@@ -37,17 +37,12 @@ const noDimensions: DimensionState = {
 
 const origin: Position = { x: 0, y: 0 };
 
-const clean = memoizeOne((phase: ?Phase): State => {
-  const state: State = {
-    // flow was not good with having a default arg on an optional type
-    phase: phase || 'IDLE',
-    drag: null,
-    drop: null,
-    dimension: noDimensions,
-  };
-
-  return state;
-});
+const clean = memoizeOne((phase?: Phase = 'IDLE'): State => ({
+  phase,
+  drag: null,
+  drop: null,
+  dimension: noDimensions,
+}));
 
 type MoveArgs = {|
   state: State,
@@ -135,18 +130,18 @@ const move = ({
 };
 
 export default (state: State = clean('IDLE'), action: Action): State => {
-  if (action.type === 'BEGIN_LIFT') {
-    if (state.phase !== 'IDLE') {
-      console.error('trying to start a lift while another is occurring');
-      return state;
-    }
-    return clean('COLLECTING_DIMENSIONS');
+  if (action.type === 'CLEAN') {
+    return clean();
+  }
+
+  if (action.type === 'PREPARE') {
+    return clean('PREPARING');
   }
 
   if (action.type === 'REQUEST_DIMENSIONS') {
-    if (state.phase !== 'COLLECTING_DIMENSIONS') {
-      console.error('trying to collect dimensions at the wrong time');
-      return state;
+    if (state.phase !== 'PREPARING') {
+      console.error('trying to start a lift while not preparing for a lift');
+      return clean();
     }
 
     const typeId: TypeId = action.payload;
@@ -538,9 +533,5 @@ export default (state: State = clean('IDLE'), action: Action): State => {
     };
   }
 
-  if (action.type === 'CLEAN') {
-    return clean();
-  }
-
   return state;
 };
diff --git a/src/types.js b/src/types.js
@@ -208,7 +208,30 @@ export type PendingDrop = {|
   result: DropResult,
 |}
 
-export type Phase = 'IDLE' | 'COLLECTING_DIMENSIONS' | 'DRAGGING' | 'DROP_ANIMATING' | 'DROP_COMPLETE';
+export type Phase =
+  // The application rest state
+  'IDLE' |
+
+  // When a drag starts we need to flush any existing animations
+  // that might be occurring. While this flush is occurring we
+  // are in this phase
+  'PREPARING' |
+
+  // After the animations have been flushed we need to collect the
+  // dimensions of all of the Draggable and Droppable components.
+  // At this point a drag has not started yet and the onDragStart
+  // hook has not fired.
+  'COLLECTING_DIMENSIONS' |
+
+  // A drag is active. The onDragStart hook has been fired
+  'DRAGGING' |
+
+  // An optional phase for animating the drop / cancel if it is needed
+  'DROP_ANIMATING' |
+
+  // The final state of a drop / cancel.
+  // This will result in the onDragEnd hook being fired
+  'DROP_COMPLETE';
 
 export type DimensionState = {|
   request: ?TypeId,