From 16c13fb07960a59894887acbceadffec9ff1e56d Mon Sep 17 00:00:00 2001 From: lukebarnard Date: Mon, 15 Jan 2018 18:12:27 +0000 Subject: [PATCH 1/6] Replace TagPanel react-dnd with react-beautiful-dnd This new library handles the simple case of an ordered vertical (or horizontal) list of items that can be reordered. It provides animations, handles positioning of items mid-drag and exposes a much simpler API to react-dnd (with a slight loss of potential function, but we don't need this flexibility here anyway). Apart from this, TagOrderStore had to be changed in a highly coupled way, but arguably for the better. Instead of being updated incrementally every time an item is dragged over another and having a separate "commit" action, the asyncronous action `moveTag` is used to reposition the tag in the list and both dispatch an optimistic update and carry out the request as before. (The MatrixActions.accountData is still used to indicate a successful reordering of tags). The view is updated instantly, in an animated way, and this is handled at the layer "above" React by the DND library. --- package.json | 1 + src/actions/TagOrderActions.js | 20 +++-- src/actions/actionCreators.js | 11 ++- src/components/structures/TagPanel.js | 42 ++++++++-- src/components/views/elements/DNDTagTile.js | 88 ++++++--------------- src/stores/TagOrderStore.js | 23 ++---- 6 files changed, 85 insertions(+), 100 deletions(-) diff --git a/package.json b/package.json index eb2cabf854..dbc27b5a08 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "querystring": "^0.2.0", "react": "^15.4.0", "react-addons-css-transition-group": "15.3.2", + "react-beautiful-dnd": "^4.0.0", "react-dnd": "^2.1.4", "react-dnd-html5-backend": "^2.1.2", "react-dom": "^15.4.0", diff --git a/src/actions/TagOrderActions.js b/src/actions/TagOrderActions.js index 60946ea7f1..db9230e964 100644 --- a/src/actions/TagOrderActions.js +++ b/src/actions/TagOrderActions.js @@ -31,16 +31,22 @@ const TagOrderActions = {}; * indicating the status of the request. * @see asyncAction */ -TagOrderActions.commitTagOrdering = function(matrixClient) { - return asyncAction('TagOrderActions.commitTagOrdering', () => { - // Only commit tags if the state is ready, i.e. not null - const tags = TagOrderStore.getOrderedTags(); - if (!tags) { - return; - } +TagOrderActions.moveTag = function(matrixClient, tag, destinationIx) { + // Only commit tags if the state is ready, i.e. not null + let tags = TagOrderStore.getOrderedTags(); + if (!tags) { + return; + } + tags = tags.filter((t) => t !== tag); + tags = [...tags.slice(0, destinationIx), tag, ...tags.slice(destinationIx)]; + + return asyncAction('TagOrderActions.moveTag', () => { Analytics.trackEvent('TagOrderActions', 'commitTagOrdering'); return matrixClient.setAccountData('im.vector.web.tag_ordering', {tags}); + }, () => { + // For an optimistic update + return {tags}; }); }; diff --git a/src/actions/actionCreators.js b/src/actions/actionCreators.js index bddfbc7c63..0238eee8c0 100644 --- a/src/actions/actionCreators.js +++ b/src/actions/actionCreators.js @@ -22,6 +22,9 @@ limitations under the License. * suffix determining whether it is pending, successful or * a failure. * @param {function} fn a function that returns a Promise. + * @param {function?} pendingFn a function that returns an object to assign + * to the `request` key of the ${id}.pending + * payload. * @returns {function} an action thunk - a function that uses its single * argument as a dispatch function to dispatch the * following actions: @@ -29,9 +32,13 @@ limitations under the License. * `${id}.success` or * `${id}.failure`. */ -export function asyncAction(id, fn) { +export function asyncAction(id, fn, pendingFn) { return (dispatch) => { - dispatch({action: id + '.pending'}); + dispatch({ + action: id + '.pending', + request: + typeof pendingFn === 'function' ? pendingFn() : undefined, + }); fn().then((result) => { dispatch({action: id + '.success', result}); }).catch((err) => { diff --git a/src/components/structures/TagPanel.js b/src/components/structures/TagPanel.js index 531c247ea6..323528e1f1 100644 --- a/src/components/structures/TagPanel.js +++ b/src/components/structures/TagPanel.js @@ -25,6 +25,8 @@ import TagOrderActions from '../../actions/TagOrderActions'; import sdk from '../../index'; import dis from '../../dispatcher'; +import { DragDropContext, Droppable } from 'react-beautiful-dnd'; + const TagPanel = React.createClass({ displayName: 'TagPanel', @@ -69,7 +71,9 @@ const TagPanel = React.createClass({ dis.dispatch(GroupActions.fetchJoinedGroups(this.context.matrixClient)); }, - onClick() { + onClick(e) { + // Ignore clicks on children + if (e.target !== e.currentTarget) return; dis.dispatch({action: 'deselect_tags'}); }, @@ -78,8 +82,20 @@ const TagPanel = React.createClass({ dis.dispatch({action: 'view_create_group'}); }, - onTagTileEndDrag() { - dis.dispatch(TagOrderActions.commitTagOrdering(this.context.matrixClient)); + onTagTileEndDrag(result) { + // Dragged to an invalid destination, not onto a droppable + if (!result.destination) { + return; + } + + // Dispatch synchronously so that the TagPanel receives an + // optimistic update from TagOrderStore before the previous + // state is shown. + dis.dispatch(TagOrderActions.moveTag( + this.context.matrixClient, + result.draggableId, + result.destination.index, + ), true); }, render() { @@ -89,16 +105,26 @@ const TagPanel = React.createClass({ const tags = this.state.orderedTags.map((tag, index) => { return ; }); return
-
- { tags } -
+ + + { (provided, snapshot) => ( +
+ { tags } + { provided.placeholder } +
+ ) } + + diff --git a/src/components/views/elements/DNDTagTile.js b/src/components/views/elements/DNDTagTile.js index 539163d0dc..e17ea52976 100644 --- a/src/components/views/elements/DNDTagTile.js +++ b/src/components/views/elements/DNDTagTile.js @@ -15,71 +15,29 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { DragSource, DropTarget } from 'react-dnd'; - import TagTile from './TagTile'; -import dis from '../../../dispatcher'; -import { findDOMNode } from 'react-dom'; -const tagTileSource = { - canDrag: function(props, monitor) { - return true; - }, +import { Draggable } from 'react-beautiful-dnd'; - beginDrag: function(props) { - // Return the data describing the dragged item - return { - tag: props.tag, - }; - }, - - endDrag: function(props, monitor, component) { - const dropResult = monitor.getDropResult(); - if (!monitor.didDrop() || !dropResult) { - return; - } - props.onEndDrag(); - }, -}; - -const tagTileTarget = { - canDrop(props, monitor) { - return true; - }, - - hover(props, monitor, component) { - if (!monitor.canDrop()) return; - const draggedY = monitor.getClientOffset().y; - const {top, bottom} = findDOMNode(component).getBoundingClientRect(); - const targetY = (top + bottom) / 2; - dis.dispatch({ - action: 'order_tag', - tag: monitor.getItem().tag, - targetTag: props.tag, - // Note: we indicate that the tag should be after the target when - // it's being dragged over the top half of the target. - after: draggedY < targetY, - }); - }, - - drop(props) { - // Return the data to be returned by getDropResult - return { - tag: props.tag, - }; - }, -}; - -export default - DropTarget('TagTile', tagTileTarget, (connect, monitor) => ({ - connectDropTarget: connect.dropTarget(), - }))(DragSource('TagTile', tagTileSource, (connect, monitor) => ({ - connectDragSource: connect.dragSource(), - }))((props) => { - const { connectDropTarget, connectDragSource, ...otherProps } = props; - return connectDropTarget(connectDragSource( -
- -
, - )); - })); +export default function DNDTagTile(props) { + return
+ + { (provided, snapshot) => ( +
+
+ +
+ { provided.placeholder } +
+ ) } + +
; +} diff --git a/src/stores/TagOrderStore.js b/src/stores/TagOrderStore.js index 9c9427284e..27ce6cbb73 100644 --- a/src/stores/TagOrderStore.js +++ b/src/stores/TagOrderStore.js @@ -78,24 +78,11 @@ class TagOrderStore extends Store { this._updateOrderedTags(); break; } - // Puts payload.tag at payload.targetTag, placing the targetTag before or after the tag - case 'order_tag': { - if (!this._state.orderedTags || - !payload.tag || - !payload.targetTag || - payload.tag === payload.targetTag - ) return; - - const tags = this._state.orderedTags; - - let orderedTags = tags.filter((t) => t !== payload.tag); - const newIndex = orderedTags.indexOf(payload.targetTag) + (payload.after ? 1 : 0); - orderedTags = [ - ...orderedTags.slice(0, newIndex), - payload.tag, - ...orderedTags.slice(newIndex), - ]; - this._setState({orderedTags}); + case 'TagOrderActions.moveTag.pending': { + // Optimistic update of a moved tag + this._setState({ + orderedTags: payload.request.tags, + }); break; } case 'select_tag': { From f7e2e91df513c409c887f3ec893ef23c77f0bc94 Mon Sep 17 00:00:00 2001 From: lukebarnard Date: Tue, 16 Jan 2018 09:38:16 +0000 Subject: [PATCH 2/6] Workaround for travis-ci/travis-ci#8836 --- .travis.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 4137d754bf..d2b5558b39 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,7 +3,9 @@ dist: trusty # we don't need sudo, so can run in a container, which makes startup much # quicker. -sudo: false +# +# unfortunately we do require sudo as per https://github.com/travis-ci/travis-ci/issues/8836 +sudo: required language: node_js node_js: From d2e5b122715bf89c1596a56eaffb1a9d3b0988d1 Mon Sep 17 00:00:00 2001 From: lukebarnard Date: Tue, 16 Jan 2018 09:46:48 +0000 Subject: [PATCH 3/6] Update jsdoc for moveTag --- src/actions/TagOrderActions.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/actions/TagOrderActions.js b/src/actions/TagOrderActions.js index db9230e964..608fd6c4c5 100644 --- a/src/actions/TagOrderActions.js +++ b/src/actions/TagOrderActions.js @@ -22,11 +22,12 @@ const TagOrderActions = {}; /** * Creates an action thunk that will do an asynchronous request to - * commit TagOrderStore.getOrderedTags() to account data and dispatch - * actions to indicate the status of the request. + * move a tag in TagOrderStore to destinationIx. * * @param {MatrixClient} matrixClient the matrix client to set the * account data on. + * @param {string} tag the tag to move. + * @param {number} destinationIx the new position of the tag. * @returns {function} an action thunk that will dispatch actions * indicating the status of the request. * @see asyncAction From f391375deac0534c9d89863e15c2ffd643b7a6de Mon Sep 17 00:00:00 2001 From: lukebarnard Date: Tue, 16 Jan 2018 10:09:11 +0000 Subject: [PATCH 4/6] Alter comment on travis-ci#8836 workaround --- .travis.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index d2b5558b39..954f14a4da 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,7 +4,8 @@ dist: trusty # we don't need sudo, so can run in a container, which makes startup much # quicker. # -# unfortunately we do require sudo as per https://github.com/travis-ci/travis-ci/issues/8836 +# unfortunately we do temporarily require sudo as a workaround for +# https://github.com/travis-ci/travis-ci/issues/8836 sudo: required language: node_js From bda2d6b0a66b4a50a1313e1aebd50c4f4555ec15 Mon Sep 17 00:00:00 2001 From: lukebarnard Date: Tue, 16 Jan 2018 10:44:11 +0000 Subject: [PATCH 5/6] Work around atlassian/react-beautiful-dnd#273 For some reason, after dragging an item the parent draggable receives a mouse click. The workaround is to use onMouseDown for deselecting tags --- src/components/structures/TagPanel.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/components/structures/TagPanel.js b/src/components/structures/TagPanel.js index 323528e1f1..a0a0e2d6e3 100644 --- a/src/components/structures/TagPanel.js +++ b/src/components/structures/TagPanel.js @@ -111,13 +111,14 @@ const TagPanel = React.createClass({ selected={this.state.selectedTags.includes(tag)} />; }); - return
+ return
{ (provided, snapshot) => (
{ tags } { provided.placeholder } From f19dcd81149b6ae0f971c3b89098fa30c559b023 Mon Sep 17 00:00:00 2001 From: lukebarnard Date: Tue, 16 Jan 2018 11:07:25 +0000 Subject: [PATCH 6/6] Comment workaround to atlassian/react-beautiful-dnd#273 --- src/components/structures/TagPanel.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/structures/TagPanel.js b/src/components/structures/TagPanel.js index a0a0e2d6e3..1cd3f04f9d 100644 --- a/src/components/structures/TagPanel.js +++ b/src/components/structures/TagPanel.js @@ -118,6 +118,10 @@ const TagPanel = React.createClass({
{ tags }