Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improved drag flow #172

Merged
merged 6 commits into from
Nov 16, 2017
Merged

Improved drag flow #172

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
837e2fb
adding an additional phase to create a more robust drag lifecycle
alexreardon Nov 15, 2017
71cfd38
adding more detail to the phase type
alexreardon Nov 15, 2017
9447b76
starting to add tests for action creators
alexreardon Nov 15, 2017
d3c91b7
adding more test cases for lift
alexreardon Nov 15, 2017
9fa0589
minor cleanup
alexreardon Nov 15, 2017
46a8ed1
fixing flow error in test
alexreardon Nov 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 37 additions & 39 deletions src/state/action-creators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,6 @@ export const requestDimensions = (type: TypeId): RequestDimensionsAction => ({
payload: type,
});

export type BeginLiftAction = {|
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no longer needed

type: 'BEGIN_LIFT'
|}

const beginLift = (): BeginLiftAction => ({
type: 'BEGIN_LIFT',
});

export type CompleteLiftAction = {|
type: 'COMPLETE_LIFT',
payload: {|
Expand All @@ -74,7 +66,7 @@ export type CompleteLiftAction = {|
|}
|}

const completeLift = (id: DraggableId,
export const completeLift = (id: DraggableId,
type: TypeId,
client: InitialDragLocation,
windowScroll: Position,
Expand Down Expand Up @@ -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: {|
Expand Down Expand Up @@ -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()));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yuck. this is gone! 🎉

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;
}
Expand Down Expand Up @@ -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') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we had no way of knowing if the drag was ended before this first timeout was fired. It is possible to do with a well orchestrated force press in touch dragging. By adding an extra phase we can know if something has changed in the elapsed time.

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 |
Expand All @@ -487,4 +484,5 @@ export type Action = BeginLiftAction |
CrossAxisMoveBackwardAction |
DropAnimateAction |
DropCompleteAction |
PrepareAction |
CleanAction;
26 changes: 11 additions & 15 deletions src/state/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ const noDimensions: DimensionState = {

const origin: Position = { x: 0, y: 0 };

const clean = memoizeOne((phase: ?Phase): State => {
const clean = memoizeOne((phase?: Phase = 'IDLE'): State => {
const state: State = {
// flow was not good with having a default arg on an optional type
phase: phase || 'IDLE',
phase,
drag: null,
drop: null,
dimension: noDimensions,
Expand Down Expand Up @@ -135,18 +135,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;
Expand Down Expand Up @@ -538,9 +538,5 @@ export default (state: State = clean('IDLE'), action: Action): State => {
};
}

if (action.type === 'CLEAN') {
return clean();
}

return state;
};
25 changes: 24 additions & 1 deletion src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' |
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new phase to allow for flushing


// 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,
Expand Down
Loading