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 16 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
6 changes: 6 additions & 0 deletions src/MatrixClientPeg.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,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 +69,8 @@ class MatrixClientPeg {

unset() {
this.matrixClient = null;

MatrixActionCreators.stop();
}

/**
Expand Down Expand Up @@ -108,6 +111,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
26 changes: 26 additions & 0 deletions src/actions/GroupActions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
Copyright 2017 Vector Creations Ltd
Copy link
Member

Choose a reason for hiding this comment

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

New Vector, fwiw


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 { createPromiseActionCreator } from './actionCreators';

const GroupActions = {};

GroupActions.fetchJoinedGroups = createPromiseActionCreator(
'GroupActions.fetchJoinedGroups',
(matrixClient) => matrixClient.getJoinedGroups(),
);

export default GroupActions;
39 changes: 39 additions & 0 deletions src/actions/MatrixActionCreators.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
Copyright 2017 Vector Creations 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 { createMatrixActionCreator } from './actionCreators';

// Events emitted from the matrixClient that we want to dispatch as actions
// via MatrixActionCreators. See createMatrixActionCreator.
const REGISTERED_EVENTS = [
"accountData",
];

export default {
actionCreators: [],
actionCreatorsStop: [],

start(matrixClient) {
this.actionCreators = REGISTERED_EVENTS.map((eventId) =>
createMatrixActionCreator(matrixClient, eventId),
);
this.actionCreatorsStop = this.actionCreators.map((ac) => ac());
Copy link
Member

Choose a reason for hiding this comment

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

I feel like I'm missing something here.

So an actioncreator is, aiui, a function which, when called, starts listening for a thing, and returns another function which will stop listening for that thing.

so we fill actionCreators with these functions which can be called to start listening for things, and then here we call all of them. And never touch this.actionCreators again. So why do we bother keeping it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OOB we decided to modify action creators so that they are all of the same structure and are either:

  • a function that takes arguments and returns an Action (JSON dict with action member) or
  • a function that returns a function (dispatch) => {...} that, when given the dispatch function, will dispatch actions asynchronously.

},

stop() {
this.actionCreatorsStop.map((stop) => stop());
},
};
33 changes: 33 additions & 0 deletions src/actions/TagOrderActions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
Copyright 2017 Vector Creations 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 { createPromiseActionCreator } from './actionCreators';
import TagOrderStore from '../stores/TagOrderStore';

const TagOrderActions = {};

TagOrderActions.commitTagOrdering = createPromiseActionCreator(
'TagOrderActions.commitTagOrdering',
(matrixClient) => {
Analytics.trackEvent('TagOrderActions', 'commitTagOrdering');
return matrixClient.setAccountData('im.vector.web.tag_ordering', {
tags: TagOrderStore.getOrderedTags(),
});
},
);

export default TagOrderActions;
72 changes: 72 additions & 0 deletions src/actions/actionCreators.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
Copyright 2017 Vector Creations 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';
Copy link
Member

Choose a reason for hiding this comment

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

is it not going to present a problem that dispatcher dispatches its actions asynchronously? I thought that screwed us over for RoomView?

Copy link
Contributor Author

@lukebarnard1 lukebarnard1 Dec 12, 2017

Choose a reason for hiding this comment

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

No, we just lose the prevention of side-effects that flux employs. Are RoomView[Store] is screwed regardless. We're only screwed if any of our dispatches assume sync when they're not sync.


/**
* Create an action creator that will dispatch actions asynchronously that
* indicate the current status of promise returned by the given function, 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 the function to call with arguments given to the
* returned function. This function should return a Promise.
* @returns {function} a function that dispatches asynchronous actions when called.
*/
export function createPromiseActionCreator(id, fn) {
return (...args) => {
dis.dispatch({action: id + '.pending'});
fn(...args).then((result) => {
dis.dispatch({action: id + '.success', result});
}).catch((err) => {
dis.dispatch({action: id + '.failure', err});
});
};
}

/**
* Create an action creator that will listen to events of type eventId emitted
* by matrixClient and dispatch a corresponding action of the following shape:
* {
* action: 'MatrixActions.' + eventId,
* event: matrixEvent,
* event_type: matrixEvent.getType(),
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 bother with event_type and event_content when the whole event is available in 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.

It simplifies the action handler slightly:

case 'MatrixActions.accountData': {
    if (payload.event_type !== 'im.vector.web.tag_ordering') break;
    this._setState({
        orderedTagsAccountData: payload.event_content ? payload.event_content.tags : null,
    });
    this._updateOrderedTags();
    break;
}

instead of putting payload.event.getContent() into a variable and then doing the conditional. Similarly for event_type but it's probably only going to be used once.

What about just sending event_type, event_content? This would also limit the amount of state that can go into the store and further limits the number of side-effects.

* event_content: matrixEvent.getContent(),
* }
* @param {MatrixClient} matrixClient the matrix client with which to register
* a listener.
* @param {string} eventId the ID of the event that hen emitted will cause the
Copy link
Member

Choose a reason for hiding this comment

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

🐔

* an action to be dispatched.
* @returns {function} a function that, when called, will begin to listen to
* dispatches from matrixClient. The result from that
* function can be called to stop listening.
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 reason the thing shouldn't start listening straight away? Then it could just return the stop function which is one less level of functions returning functions for me to keep track of.

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 only reason is that it wouldn't really be an Action Creator if you didn't have to call it to make it start dispatching things.

*/
export function createMatrixActionCreator(matrixClient, eventId) {
Copy link
Member

Choose a reason for hiding this comment

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

as per #riot-dev, I think that, for now at least, it's going to be clearer if you make this specific to accountData rather than trying to make it generic for all matrixClient-emitted events.

const listener = (matrixEvent) => {
dis.dispatch({
action: 'MatrixActions.' + eventId,
event: matrixEvent,
event_type: matrixEvent.getType(),
event_content: matrixEvent.getContent(),
});
};
return () => {
matrixClient.on(eventId, listener);
return () => {
matrixClient.removeListener(eventId, listener);
};
};
}
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