From 212365b72111d80857dd7c51209d7bed631986bd Mon Sep 17 00:00:00 2001 From: Alex Reardon Date: Fri, 15 Jun 2018 10:29:05 +1000 Subject: [PATCH] moving forward --- src/debug/timings.js | 2 +- src/state/action-creators.js | 10 +- .../dimension-marshal-types.js | 4 +- .../dimension-marshal/dimension-marshal.js | 29 +--- src/state/dimension-marshal/publisher.js | 66 +++++--- .../get-dimension-map.js | 27 +-- .../get-drag-positions.js | 0 .../{publish => publish-change}/index.js | 8 +- src/state/reducer.js | 6 +- src/state/to-map.js | 18 ++ src/types.js | 8 +- .../drag-drop-context/drag-drop-context.jsx | 4 +- .../dimension-marshal/initial-publish.spec.js | 157 ++++++++++++++++++ .../dimension-marshal/publish-change.spec.js | 37 +++++ test/unit/view/dimension-marshal/util.js | 4 +- test/utils/preset-action-args.js | 11 +- 16 files changed, 300 insertions(+), 91 deletions(-) rename src/state/{publish => publish-change}/get-dimension-map.js (88%) rename src/state/{publish => publish-change}/get-drag-positions.js (100%) rename src/state/{publish => publish-change}/index.js (97%) create mode 100644 src/state/to-map.js create mode 100644 test/unit/view/dimension-marshal/initial-publish.spec.js create mode 100644 test/unit/view/dimension-marshal/publish-change.spec.js diff --git a/src/debug/timings.js b/src/debug/timings.js index cf0abc34db..b0b085fd3c 100644 --- a/src/debug/timings.js +++ b/src/debug/timings.js @@ -11,7 +11,7 @@ const flag: string = '__react-beautiful-dnd-debug-timings-hook__'; const isTimingsEnabled = (): boolean => Boolean(window[flag]); // Debug: uncomment to enable -window[flag] = true; +// window[flag] = true; export const start = (key: string) => { if (!isTimingsEnabled()) { diff --git a/src/state/action-creators.js b/src/state/action-creators.js index 93eef33f4e..6eadbea21a 100644 --- a/src/state/action-creators.js +++ b/src/state/action-creators.js @@ -11,7 +11,7 @@ import type { DimensionMap, DropReason, PendingDrop, - Publish, + PublishChange, } from '../types'; export type LiftArgs = {| @@ -51,12 +51,12 @@ export const initialPublish = (args: InitialPublishArgs): InitialPublishAction = payload: args, }); -export type PublishAction = {| +export type PublishChangeAction = {| type: 'PUBLISH', - payload: Publish + payload: PublishChange |} -export const publish = (args: Publish): PublishAction => ({ +export const publishChange = (args: PublishChange): PublishChangeAction => ({ type: 'PUBLISH', payload: args, }); @@ -250,7 +250,7 @@ export const dropAnimationFinished = (): DropAnimationFinishedAction => ({ export type Action = LiftAction | InitialPublishAction | - PublishAction | + PublishChangeAction | CollectionStartingAction | UpdateDroppableScrollAction | UpdateDroppableIsEnabledAction | diff --git a/src/state/dimension-marshal/dimension-marshal-types.js b/src/state/dimension-marshal/dimension-marshal-types.js index c28207a7e7..7287b8f46f 100644 --- a/src/state/dimension-marshal/dimension-marshal-types.js +++ b/src/state/dimension-marshal/dimension-marshal-types.js @@ -17,7 +17,7 @@ import type { Critical, DimensionMap, LiftRequest, - Publish, + PublishChange, } from '../../types'; export type GetDraggableDimensionFn = ( @@ -109,7 +109,7 @@ export type DimensionMarshal = {| |} export type Callbacks = {| - publish: (args: Publish) => void, + publishChange: (args: PublishChange) => void, updateDroppableScroll: (args: UpdateDroppableScrollArgs) => void, updateDroppableIsEnabled: (args: UpdateDroppableIsEnabledArgs) => void, collectionStarting: () => void, diff --git a/src/state/dimension-marshal/dimension-marshal.js b/src/state/dimension-marshal/dimension-marshal.js index 3f21c38ef7..3eabcd99d1 100644 --- a/src/state/dimension-marshal/dimension-marshal.js +++ b/src/state/dimension-marshal/dimension-marshal.js @@ -31,33 +31,6 @@ import type { } from './dimension-marshal-types'; export default (callbacks: Callbacks) => { - const advancedUsageWarning = (() => { - let hasAnnounced: boolean = false; - - return () => { - if (hasAnnounced) { - return; - } - - hasAnnounced = true; - - if (process.env.NODE_ENV !== 'production') { - console.warn(` - Advanced usage warning: you are triggering a recollection of dimensions during a drag. - This an advanced feature used to support dynamic interactions such as lazy loading lists. - You might not have intended to trigger this collection. A collection will be triggered - when: - - - A Draggable or Droppable is added or removed - - Draggable: 'id' or 'index' change - - Droppable: 'id' change ('type' change is not permitted during a drag) - - (This warning will be stripped in production) - `.trim()); - } - }; - })(); - const entries: Entries = { droppables: {}, draggables: {}, @@ -66,7 +39,7 @@ export default (callbacks: Callbacks) => { const publisher: Publisher = createPublisher({ callbacks: { - publish: callbacks.publish, + publishChange: callbacks.publishChange, collectionStarting: callbacks.collectionStarting, }, getProvided: (): Provided => { diff --git a/src/state/dimension-marshal/publisher.js b/src/state/dimension-marshal/publisher.js index db51d296f0..7dfade2c13 100644 --- a/src/state/dimension-marshal/publisher.js +++ b/src/state/dimension-marshal/publisher.js @@ -3,11 +3,9 @@ import type { Position } from 'css-box-model'; import type { DraggableId, DroppableId, - Publish, + PublishChange, DraggableDimension, DroppableDimension, - DraggableDimensionMap, - DroppableDimensionMap, } from '../../types'; import type { Collection, Entries } from './dimension-marshal-types'; import * as timings from '../../debug/timings'; @@ -39,7 +37,7 @@ export type Provided = {| |} type Callbacks = {| - publish: (args: Publish) => void, + publishChange: (args: PublishChange) => void, collectionStarting: () => void, |} @@ -55,6 +53,37 @@ const getEmptyMap = (): Map => ({ const timingKey: string = 'Publish collection from DOM'; +const advancedUsageWarning = (() => { + // noop for production + if (process.env.NODE_ENV === 'production') { + return () => { }; + } + + let hasAnnounced: boolean = false; + + return () => { + if (hasAnnounced) { + return; + } + + hasAnnounced = true; + + console.warn(` + Advanced usage warning: you are adding or removing a dimension during a drag + This an advanced feature used to support dynamic interactions such as lazy loading lists. + + Keep in mind the following restrictions: + + - Draggable's can only be added to Droppable's that are scroll containers + - Adding a Droppable cannot impact the placement of other Droppables + (it cannot push a Droppable on the page) + + (This warning will be stripped in production builds) + `.trim() + ); + }; +})(); + export default ({ getProvided, callbacks, @@ -69,6 +98,8 @@ export default ({ }; const collect = () => { + advancedUsageWarning(); + if (frameId) { return; } @@ -81,31 +112,26 @@ export default ({ const { entries, collection } = getProvided(); const windowScroll: Position = collection.initialWindowScroll; - const draggables: DraggableDimensionMap = Object.keys(additions.draggables) + const draggables: DraggableDimension[] = Object.keys(additions.draggables) .map((id: DraggableId): DraggableDimension => // TODO entries.draggables[id].getDimension(windowScroll, { x: 0, y: 0 }) - ) - .reduce((previous, current) => { - previous[current.descriptor.id] = current; - return previous; - }, {}); + ); - const droppables: DroppableDimensionMap = Object.keys(additions.droppables) + const droppables: DroppableDimension[] = Object.keys(additions.droppables) .map((id: DroppableId): DroppableDimension => entries.droppables[id].callbacks.getDimensionAndWatchScroll( // TODO: need to figure out diff from start? windowScroll, collection.scrollOptions ) - ) - .reduce((previous, current) => { - previous[current.descriptor.id] = current; - return previous; - }, {}); - - const result: Publish = { - additions: { draggables, droppables }, + ); + + const result: PublishChange = { + additions: { + draggables, + droppables, + }, removals: { draggables: Object.keys(removals.draggables), droppables: Object.keys(removals.droppables), @@ -115,7 +141,7 @@ export default ({ reset(); timings.finish(timingKey); - callbacks.publish(result); + callbacks.publishChange(result); }); }; diff --git a/src/state/publish/get-dimension-map.js b/src/state/publish-change/get-dimension-map.js similarity index 88% rename from src/state/publish/get-dimension-map.js rename to src/state/publish-change/get-dimension-map.js index ab01215741..861b687adc 100644 --- a/src/state/publish/get-dimension-map.js +++ b/src/state/publish-change/get-dimension-map.js @@ -9,19 +9,21 @@ import { import type { Axis, DimensionMap, - Publish, + PublishChange, DraggableId, DroppableId, DraggableDimension, DroppableDimension, DraggableDimensionMap, + DroppableDimensionMap, } from '../../types'; +import { toDroppableMap, toDraggableMap } from '../to-map'; import { patch } from '../position'; import * as timings from '../../debug/timings'; type Args = {| existing: DimensionMap, - publish: Publish, + publishChange: PublishChange, windowScroll: Position, |} @@ -77,16 +79,17 @@ const timingKey: string = 'Dynamic dimension change processing (just math)'; export default ({ existing, - publish, + publishChange, windowScroll, }: Args): DimensionMap => { timings.start(timingKey); - const partitioned: Partitioned = Object.keys(publish.additions.draggables) - .map((id: DraggableId): DraggableDimension => publish.additions.draggables[id]) + const addedDroppables: DroppableDimensionMap = toDroppableMap(publishChange.additions.droppables); + const addedDraggables: DraggableDimensionMap = toDraggableMap(publishChange.additions.draggables); + + const partitioned: Partitioned = publishChange.additions.draggables .reduce((previous: Partitioned, draggable: DraggableDimension) => { const droppableId: DroppableId = draggable.descriptor.droppableId; - const isInNewDroppable: boolean = - Boolean(publish.additions.droppables[droppableId]); + const isInNewDroppable: boolean = Boolean(addedDroppables[droppableId]); if (isInNewDroppable) { previous.inNewDroppable.push(draggable); @@ -116,7 +119,7 @@ export default ({ }); // Draggable removals - publish.removals.draggables.forEach((id: DraggableId) => { + publishChange.removals.draggables.forEach((id: DraggableId) => { // Pull draggable dimension from existing dimensions const draggable: ?DraggableDimension = existing.draggables[id]; invariant(draggable, `Cannot find Draggable ${id}`); @@ -195,21 +198,21 @@ export default ({ const dimensions: DimensionMap = { draggables: { ...shifted, - ...publish.additions.draggables, + ...addedDraggables, }, droppables: { ...existing.droppables, - ...publish.additions.droppables, + ...addedDroppables, }, }; // We also need to remove the Draggables and Droppables from this new map - publish.removals.draggables.forEach((id: DraggableId) => { + publishChange.removals.draggables.forEach((id: DraggableId) => { delete dimensions.draggables[id]; }); - publish.removals.droppables.forEach((id: DroppableId) => { + publishChange.removals.droppables.forEach((id: DroppableId) => { delete dimensions.droppables[id]; }); diff --git a/src/state/publish/get-drag-positions.js b/src/state/publish-change/get-drag-positions.js similarity index 100% rename from src/state/publish/get-drag-positions.js rename to src/state/publish-change/get-drag-positions.js diff --git a/src/state/publish/index.js b/src/state/publish-change/index.js similarity index 97% rename from src/state/publish/index.js rename to src/state/publish-change/index.js index fe20590a2e..b461d08bd4 100644 --- a/src/state/publish/index.js +++ b/src/state/publish-change/index.js @@ -6,7 +6,7 @@ import type { DraggingState, CollectingState, DropPendingState, - Publish, + PublishChange, Critical, DraggableId, DraggableDimension, @@ -18,12 +18,12 @@ import getDragPositions from './get-drag-positions'; type Args = {| state: CollectingState | DropPendingState, - publish: Publish + publishChange: PublishChange |} export default ({ state, - publish, + publishChange, }: Args): DraggingState | DropPendingState => { // TODO: write validate that every removed draggable must have a removed droppable @@ -49,7 +49,7 @@ export default ({ const dimensions: DimensionMap = getDimensionMap({ existing: state.dimensions, - publish, + publishChange, // TODO: fix windowScroll: state.viewport.scroll.initial, }); diff --git a/src/state/reducer.js b/src/state/reducer.js index e2d452f9da..780e1d7ad1 100644 --- a/src/state/reducer.js +++ b/src/state/reducer.js @@ -6,7 +6,7 @@ import getDragImpact from './get-drag-impact/'; import moveCrossAxis from './move-cross-axis/'; import moveToNextIndex from './move-to-next-index/'; import { noMovement } from './no-impact'; -import publish from './publish'; +import publishChange from './publish-change'; import { add, isEqual, subtract } from './position'; import scrollViewport from './scroll-viewport'; import getHomeImpact from './get-home-impact'; @@ -184,9 +184,9 @@ export default (state: State = idle, action: Action): State => { `Unexpected ${action.type} received in phase ${state.phase}` ); - return publish({ + return publishChange({ state, - publish: action.payload, + publishChange: action.payload, }); } diff --git a/src/state/to-map.js b/src/state/to-map.js new file mode 100644 index 0000000000..7b8ba3f32e --- /dev/null +++ b/src/state/to-map.js @@ -0,0 +1,18 @@ +// @flow +import type { + DroppableDimension, + DroppableDimensionMap, + DraggableDimension, + DraggableDimensionMap, +} from '../types'; + +const reduce = (dimensions: any) => dimensions.reduce((previous, current) => { + previous[current.descriptor.id] = current; + return previous; +}, {}); + +export const toDroppableMap = + (droppables: DroppableDimension[]): DroppableDimensionMap => reduce(droppables); + +export const toDraggableMap = + (draggables: DraggableDimension[]): DraggableDimensionMap => reduce(draggables); diff --git a/src/types.js b/src/types.js index 70c898e8b2..f664fdbae3 100644 --- a/src/types.js +++ b/src/types.js @@ -257,8 +257,12 @@ export type DimensionMap = {| droppables: DroppableDimensionMap, |} -export type Publish = {| - additions: DimensionMap, +export type PublishChange = {| + additions: {| + draggables: DraggableDimension[], + droppables: DroppableDimension[], + |}, + // additions: DimensionMap, removals: {| draggables: DraggableId[], droppables: DroppableId[], diff --git a/src/view/drag-drop-context/drag-drop-context.jsx b/src/view/drag-drop-context/drag-drop-context.jsx index 85d4d0cad5..508a9d7527 100644 --- a/src/view/drag-drop-context/drag-drop-context.jsx +++ b/src/view/drag-drop-context/drag-drop-context.jsx @@ -32,7 +32,7 @@ import { import { clean, move, - publish, + publishChange, updateDroppableScroll, updateDroppableIsEnabled, collectionStarting, @@ -93,7 +93,7 @@ export default class DragDropContext extends React.Component { }); const callbacks: DimensionMarshalCallbacks = bindActionCreators({ collectionStarting, - publish, + publishChange, updateDroppableScroll, updateDroppableIsEnabled, }, this.store.dispatch); diff --git a/test/unit/view/dimension-marshal/initial-publish.spec.js b/test/unit/view/dimension-marshal/initial-publish.spec.js new file mode 100644 index 0000000000..379be1cc13 --- /dev/null +++ b/test/unit/view/dimension-marshal/initial-publish.spec.js @@ -0,0 +1,157 @@ +// @flow +import createDimensionMarshal from '../../../../src/state/dimension-marshal/dimension-marshal'; +import { getPreset } from '../../../utils/dimension'; +import type { + Callbacks, + DimensionMarshal, + DroppableCallbacks, + StartPublishingResult, +} from '../../../../src/state/dimension-marshal/dimension-marshal-types'; +import getViewport from '../../../../src/view/window/get-viewport'; +import type { + LiftRequest, + DraggableDimension, + DroppableDimension, + DimensionMap, +} from '../../../../src/types'; +import { + critical, copy, +} from '../../../utils/preset-action-args'; +import { + populateMarshal, + getDroppableCallbacks, + withExpectedAdvancedUsageWarning, + getCallbacksStub, +} from './util'; + +const preset = getPreset(); + +const defaultRequest: LiftRequest = { + draggableId: critical.draggable.id, + scrollOptions: { + shouldPublishImmediately: false, + }, +}; + +const foreignWithNewType: DroppableDimension = { + ...preset.foreign, + descriptor: { + ...preset.foreign.descriptor, + id: 'new foreign id', + type: 'some cool new type', + }, +}; + +const inForeignWithNewType: DraggableDimension = { + ...preset.inForeign1, + descriptor: { + ...preset.inForeign1.descriptor, + id: 'new in foreign 1 id', + type: foreignWithNewType.descriptor.type, + }, +}; + +const withNewType: DimensionMap = { + draggables: { + ...preset.dimensions.draggables, + [inForeignWithNewType.descriptor.id]: inForeignWithNewType, + }, + droppables: { + ...preset.dimensions.droppables, + [foreignWithNewType.descriptor.id]: foreignWithNewType, + }, +}; + +it('should publish the registered dimensions (simple)', () => { + const marshal: DimensionMarshal = createDimensionMarshal(getCallbacksStub()); + + marshal.registerDraggable(preset.inHome1.descriptor, () => preset.inHome1); + marshal.registerDraggable(preset.inHome2.descriptor, () => preset.inHome2); + + const droppableCallbacks: DroppableCallbacks = getDroppableCallbacks(preset.home); + marshal.registerDroppable(preset.home.descriptor, droppableCallbacks); + + const result: StartPublishingResult = + marshal.startPublishing(defaultRequest, preset.windowScroll); + const expected: StartPublishingResult = { + critical, + dimensions: { + draggables: { + [preset.inHome1.descriptor.id]: preset.inHome1, + [preset.inHome2.descriptor.id]: preset.inHome2, + }, + droppables: { + [preset.home.descriptor.id]: preset.home, + }, + }, + }; + expect(expected).toEqual(result); +}); + +// Just checking our preset behaves how we expect +it('should publish the registered dimensions (preset)', () => { + const marshal: DimensionMarshal = createDimensionMarshal(getCallbacksStub()); + populateMarshal(marshal); + + const result: StartPublishingResult = + marshal.startPublishing(defaultRequest, preset.windowScroll); + + expect(result).toEqual({ + critical, + dimensions: preset.dimensions, + }); +}); + +it('should not publish dimensions that do not have the same type as the critical droppable', () => { + const marshal: DimensionMarshal = createDimensionMarshal(getCallbacksStub()); + populateMarshal(marshal, withNewType); + + const result: StartPublishingResult = + marshal.startPublishing(defaultRequest, preset.windowScroll); + + expect(result).toEqual({ + critical, + // dimensions with new type not gathered + dimensions: preset.dimensions, + }); +}); + +it('should not publish dimensions that have been unregistered', () => { + const marshal: DimensionMarshal = createDimensionMarshal(getCallbacksStub()); + populateMarshal(marshal); + const expectedMap: DimensionMap = copy(preset.dimensions); + + marshal.unregisterDraggable(preset.inHome2.descriptor); + delete expectedMap.draggables[preset.inHome2.descriptor.id]; + + marshal.unregisterDroppable(preset.foreign.descriptor); + delete expectedMap.droppables[preset.foreign.descriptor.id]; + + // Being a good citizen and also unregistering all of the children + preset.inForeignList.forEach((draggable: DraggableDimension) => { + marshal.unregisterDraggable(draggable.descriptor); + delete expectedMap.draggables[draggable.descriptor.id]; + }); + + const result: StartPublishingResult = + marshal.startPublishing(defaultRequest, preset.windowScroll); + + expect(result).toEqual({ + critical, + dimensions: expectedMap, + }); +}); + +it('should publish dimensions that have been updated', () => { + +}); + +describe('subsequent calls', () => { + it('should return dimensions a subsequent call', () => { + + }); + + it('should account for changes after the last call', () => { + + }); +}); diff --git a/test/unit/view/dimension-marshal/publish-change.spec.js b/test/unit/view/dimension-marshal/publish-change.spec.js new file mode 100644 index 0000000000..87279d2f28 --- /dev/null +++ b/test/unit/view/dimension-marshal/publish-change.spec.js @@ -0,0 +1,37 @@ +// @flow + +describe('additions', () => { + it('should collect and publish the dimension', () => { + + }); + + it('should not collect a dimension that does not have the same type as the dragging item', () => { + + }); +}); + +describe('removals', () => { + it('should publish a removal', () => { + + }); + + it('should not publish a removal when the dimension type is not the same as the dragging item', () => { + + }); + + it('should throw an error if trying to remove a critical dimension', () => { + + }); +}); + +describe('batching', () => { + it('should batch multiple changes within an animation frame into a single collection and publish', () => { + + }); +}); + +describe('cancelling', () => { + it('should cancel any pending collections', () => { + + }); +}); diff --git a/test/unit/view/dimension-marshal/util.js b/test/unit/view/dimension-marshal/util.js index 4781008e8e..88acaf16d4 100644 --- a/test/unit/view/dimension-marshal/util.js +++ b/test/unit/view/dimension-marshal/util.js @@ -106,8 +106,8 @@ export const withExpectedAdvancedUsageWarning = (fn: Function) => { }; export const getCallbacksStub = (): Callbacks => ({ - bulkReplace: jest.fn(), + publish: jest.fn(), updateDroppableScroll: jest.fn(), updateDroppableIsEnabled: jest.fn(), - bulkCollectionStarting: jest.fn(), + collectionStarting: jest.fn(), }); diff --git a/test/utils/preset-action-args.js b/test/utils/preset-action-args.js index 73d70824af..4abaa831c1 100644 --- a/test/utils/preset-action-args.js +++ b/test/utils/preset-action-args.js @@ -56,18 +56,9 @@ export const liftArgs: LiftArgs = { autoScrollMode: 'FLUID', }; -export const criticalDimensions: DimensionMap = { - draggables: { - [critical.draggable.id]: preset.inHome1, - }, - droppables: { - [preset.home.descriptor.id]: preset.home, - }, -}; - export const initialPublishArgs: InitialPublishArgs = { critical, - dimensions: criticalDimensions, + dimensions: preset.dimensions, client, viewport, autoScrollMode: 'FLUID',