Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

DnD Ordered TagPanel #1653

Merged
merged 39 commits into from
Dec 14, 2017
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
8178496
Implement Store for ordering tags in the tag panel
lukebarnard1 Dec 5, 2017
82a95f0
Simplify order_tag in TagOrderStore
lukebarnard1 Dec 6, 2017
a8a650c
Move TagTile to separate file, and make it dragable
lukebarnard1 Dec 6, 2017
7aa5dce
Move DragDropContext to wrap entire app
lukebarnard1 Dec 6, 2017
4af7def
Use AccountData im.vector.web.tag_ordering
lukebarnard1 Dec 6, 2017
35a108e
Simplify render of TagPanel - remove sorting
lukebarnard1 Dec 6, 2017
a9cc8eb
Remove redundant TagOrderStore.orderedTags setting
lukebarnard1 Dec 6, 2017
8f88995
Add analytics to TagOrderStore
lukebarnard1 Dec 6, 2017
7e1f1cd
Move DragDropContext to wrap LoggedInView
lukebarnard1 Dec 6, 2017
65d8833
Fix linting
lukebarnard1 Dec 6, 2017
ee6df10
Introduce action creators
lukebarnard1 Dec 7, 2017
1251544
Handle accountData events from TagOrderStore
lukebarnard1 Dec 7, 2017
7255096
Move 'commit_tags' to action creator
lukebarnard1 Dec 8, 2017
31a52c1
Fix bug with removing matrix listeners
lukebarnard1 Dec 8, 2017
53e7232
Linting
lukebarnard1 Dec 8, 2017
8f07744
Remove redundant MatrixChat
lukebarnard1 Dec 8, 2017
df88b71
Comment typo
lukebarnard1 Dec 8, 2017
991ea4e
Fix a few bugs with TagOrderStore:
lukebarnard1 Dec 11, 2017
aa91409
Return null if TagOrderStore is loading
lukebarnard1 Dec 11, 2017
0b38bf5
Do not allow ordering until TagOrderStore has loaded
lukebarnard1 Dec 11, 2017
8d2d3e6
Only commit a non-falsy tags list
lukebarnard1 Dec 11, 2017
a120335
Handle groups being joined and left
lukebarnard1 Dec 11, 2017
3e532e3
Use consistent indentation and break;s in TagOrderStore switch
lukebarnard1 Dec 12, 2017
60d8ebb
Refactor MatrixActions to something much easier to grok.
lukebarnard1 Dec 12, 2017
13925db
Refactor to allow dispatching of two kinds of Actions
lukebarnard1 Dec 12, 2017
d5534a9
Copyright
lukebarnard1 Dec 13, 2017
cc30b8f
Doc MatrixActionCreators properly
lukebarnard1 Dec 13, 2017
5de0559
Adjust actionCreators doc
lukebarnard1 Dec 13, 2017
42c1f3c
Fix incorrect bind
lukebarnard1 Dec 13, 2017
a8b245d
Add unmounted guard
lukebarnard1 Dec 13, 2017
f38690f
Doc orderedGroupTagProfiles
lukebarnard1 Dec 13, 2017
e1ea8f0
Copy state when initialisng, reset state when logging out
lukebarnard1 Dec 13, 2017
a653ece
Doc commitTagOrdering
lukebarnard1 Dec 13, 2017
ddf5dba
Doc fetchJoinedGroups
lukebarnard1 Dec 13, 2017
31ea092
Improve createAccountDataAction docs
lukebarnard1 Dec 13, 2017
fe6b7c0
Improve _addMatrixClientListener docs
lukebarnard1 Dec 13, 2017
950f591
Clarify more docs
lukebarnard1 Dec 13, 2017
6b02f59
Spelling
lukebarnard1 Dec 13, 2017
629cd13
Even better docs
lukebarnard1 Dec 13, 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
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@
"querystring": "^0.2.0",
"react": "^15.4.0",
"react-addons-css-transition-group": "15.3.2",
"react-dnd": "^2.1.4",
"react-dnd-html5-backend": "^2.1.2",
"react-dom": "^15.4.0",
"react-gemini-scrollbar": "matrix-org/react-gemini-scrollbar#5e97aef",
"sanitize-html": "^1.14.1",
Expand Down
7 changes: 7 additions & 0 deletions src/MatrixClientPeg.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
Copyright 2015, 2016 OpenMarket Ltd
Copyright 2017 Vector Creations Ltd.
Copyright 2017 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -22,6 +23,7 @@ import EventTimeline from 'matrix-js-sdk/lib/models/event-timeline';
import EventTimelineSet from 'matrix-js-sdk/lib/models/event-timeline-set';
import createMatrixClient from './utils/createMatrixClient';
import SettingsStore from './settings/SettingsStore';
import MatrixActionCreators from './actions/MatrixActionCreators';

interface MatrixClientCreds {
homeserverUrl: string,
Expand Down Expand Up @@ -68,6 +70,8 @@ class MatrixClientPeg {

unset() {
this.matrixClient = null;

MatrixActionCreators.stop();
}

/**
Expand Down Expand Up @@ -108,6 +112,9 @@ class MatrixClientPeg {
// regardless of errors, start the client. If we did error out, we'll
// just end up doing a full initial /sync.

// Connect the matrix client to the dispatcher
MatrixActionCreators.start(this.matrixClient);

console.log(`MatrixClientPeg: really starting MatrixClient`);
this.get().startClient(opts);
console.log(`MatrixClientPeg: MatrixClient started`);
Expand Down
33 changes: 33 additions & 0 deletions src/actions/GroupActions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
Copyright 2017 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import { asyncAction } from './actionCreators';

const GroupActions = {};

/**
* Create a GroupActions.fetchJoinedGroups action that represents an
Copy link
Member

Choose a reason for hiding this comment

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

not aiui.

It returns an action creator: ie, a function which, when called, will start such a request and create such a (set of) actions.

Copy link
Member

Choose a reason for hiding this comment

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

this is still incorrect

* asyncronous request to fetch the groups to which a user is joined.
Copy link
Member

Choose a reason for hiding this comment

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

asynchronous

*
* @param {MatrixClient} matrixClient the matrix client to query.
* @returns {function} an asyncronous action of type
* GroupActions.fetchJoinedGroups.
*/
GroupActions.fetchJoinedGroups = function(matrixClient) {
Copy link
Member

Choose a reason for hiding this comment

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

needs more docs

return asyncAction('GroupActions.fetchJoinedGroups', () => matrixClient.getJoinedGroups());
};

export default GroupActions;
98 changes: 98 additions & 0 deletions src/actions/MatrixActionCreators.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
Copyright 2017 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import dis from '../dispatcher';

// TODO: migrate from sync_state to MatrixActions.sync so that more js-sdk events
// become dispatches in the same place.
/**
* Create a MatrixActions.sync action that represents a MatrixClient `sync` event.
Copy link
Member

Choose a reason for hiding this comment

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

yup, we should still document the members of the action object imho.

*
* @param {MatrixClient} matrixClient the matrix client
* @param {string} state the current sync state.
* @param {string} prevState the previous sync state.
* @returns {Object} an action of type MatrixActions.sync.
*/
function createSyncAction(matrixClient, state, prevState) {
return {
action: 'MatrixActions.sync',
state,
prevState,
matrixClient,
};
}

/**
* Create a MatrixActions.accountData action that represents a MatrixClient `accountData`
* matrix event.
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

(it would help my brain if you could give some examples of what event_type is likely to be, here)

*
* @param {MatrixClient} matrixClient the matrix client.
Copy link
Member

Choose a reason for hiding this comment

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

why do we pass this in, given it's ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically so that we can handle both action creators the same in _addMatrixClientListener

* @param {MatrixEvent} accountDataEvent the account data event.
* @returns {Object} an action of type MatrixActions.accountData.
*/
function createAccountDataAction(matrixClient, accountDataEvent) {
return {
action: 'MatrixActions.accountData',
event: accountDataEvent,
event_type: accountDataEvent.getType(),
event_content: accountDataEvent.getContent(),
};
}

/**
* This object is responsible for dispatching actions when certain events are emitted by
* the given MatrixClient.
*/
export default {
// A list of callbacks to call to unregister all listeners added
_matrixClientListenersStop: [],
Copy link
Member

Choose a reason for hiding this comment

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

could probably do with a comment here too


/**
* Start listening to certain events from the MatrixClient and dispatch actions when
* they are emitted.
* @param {MatrixClient} matrixClient the MatrixClient to listen to events from
*/
start(matrixClient) {
this._addMatrixClientListener(matrixClient, 'sync', createSyncAction);
this._addMatrixClientListener(matrixClient, 'accountData', createAccountDataAction);
},

/**
* Start listening to events emitted by matrixClient, dispatch an action created by the
Copy link
Member

Choose a reason for hiding this comment

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

is there a word missing here?

* actionCreator function.
* @param {MatrixClient} matrixClient a MatrixClient to register a listener with.
* @param {string} eventName the event to listen to on MatrixClient.
* @param {function} actionCreator a function that should return an action to dispatch
Copy link
Member

Choose a reason for hiding this comment

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

could you also specify that the function will be passed the MatrixClient event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming you meant MatrixClient itself

Copy link
Member

Choose a reason for hiding this comment

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

both, but yes.

* when given the arguments emitted in the MatrixClient
* event.
*/
_addMatrixClientListener(matrixClient, eventName, actionCreator) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this is internal, but it's opaque enough that it could do with some comments

const listener = (...args) => {
dis.dispatch(actionCreator(matrixClient, ...args));
};
matrixClient.on(eventName, listener);
this._matrixClientListenersStop.push(() => {
matrixClient.removeListener(eventName, listener);
});
},

/**
* Stop listening to events.
*/
stop() {
this._matrixClientListenersStop.forEach((stopListener) => stopListener());
},
};
46 changes: 46 additions & 0 deletions src/actions/TagOrderActions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
Copyright 2017 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

import Analytics from '../Analytics';
import { asyncAction } from './actionCreators';
import TagOrderStore from '../stores/TagOrderStore';

const TagOrderActions = {};

/**
* Create a TagOrderActions.commitTagOrdering action that represents an
Copy link
Member

Choose a reason for hiding this comment

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

as with GroupActions.fetchJoinedGroups, I don't think this is correct.

* asyncronous request to commit TagOrderStore.getOrderedTags() to account
* data.
*
* @param {MatrixClient} matrixClient the matrix client to set the account
* data on.
* @returns {function} an asyncronous action of type
* TagOrderActions.commitTagOrdering.
*/
TagOrderActions.commitTagOrdering = function(matrixClient) {
Copy link
Member

Choose a reason for hiding this comment

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

needs docs

return asyncAction('TagOrderActions.commitTagOrdering', () => {
// Only commit tags if the state is ready, i.e. not null
const tags = TagOrderStore.getOrderedTags();
if (!tags) {
return;
}

Analytics.trackEvent('TagOrderActions', 'commitTagOrdering');
return matrixClient.setAccountData('im.vector.web.tag_ordering', {tags});
});
};

export default TagOrderActions;
36 changes: 36 additions & 0 deletions src/actions/actionCreators.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
Copyright 2017 New Vector Ltd

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

/**
* Create an asynchronous action creator that will dispatch actions indicating
* the current status of the promise returned by fn.
* @param {string} id the id to give the dispatched actions. This is given a
* suffix determining whether it is pending, successful or
* a failure.
* @param {function} fn a function that returns a Promise.
* @returns {function} an asyncronous action creator - a function that uses its
* single argument as a dispatch function.
*/
export function asyncAction(id, fn) {
return (dispatch) => {
dispatch({action: id + '.pending'});
fn().then((result) => {
dispatch({action: id + '.success', result});
}).catch((err) => {
dispatch({action: id + '.failure', err});
});
};
}
6 changes: 5 additions & 1 deletion src/components/structures/LoggedInView.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ limitations under the License.

import * as Matrix from 'matrix-js-sdk';
import React from 'react';
import { DragDropContext } from 'react-dnd';
import HTML5Backend from 'react-dnd-html5-backend';

import { KeyCode, isOnlyCtrlOrCmdKeyEvent } from '../../Keyboard';
import Notifier from '../../Notifier';
Expand All @@ -38,7 +40,7 @@ import SettingsStore from "../../settings/SettingsStore";
*
* Components mounted below us can access the matrix client via the react context.
*/
export default React.createClass({
const LoggedInView = React.createClass({
displayName: 'LoggedInView',

propTypes: {
Expand Down Expand Up @@ -344,3 +346,5 @@ export default React.createClass({
);
},
});

export default DragDropContext(HTML5Backend)(LoggedInView);
Copy link
Member

Choose a reason for hiding this comment

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

I know little about react-dnd but I assume this still allows us to have other drag-n-drop contexts within it (ie. is it going to break the RoomTile DND?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of moving it to the root LoggedInView is such that we can actually do DnD wherever we please. The instructions said to wrap the entire app, using two contexts (on e.g. TagPanel and LeftPanel) causes bugs.

Copy link
Member

Choose a reason for hiding this comment

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

ok 👍

2 changes: 1 addition & 1 deletion src/components/structures/MatrixChat.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ const ONBOARDING_FLOW_STARTERS = [
'view_create_group',
];

module.exports = React.createClass({
export default React.createClass({
// we export this so that the integration tests can use it :-S
statics: {
VIEWS: VIEWS,
Expand Down
Loading