DnD Ordered TagPanel #1653

lukebarnard1 · 2017-12-06T14:25:33Z

Add ability to reorder the TagPanel items.

This works by having a TagOrderStore that stores the tags shown in the TagPanel in an order that the user controls via DnD. The full list of tags is stored in the im.vector.web.tag_ordering account event. By default, the ordering is alphabetical.

such that: - it takes a targetTag to be replaced instead the previous tag to insert after - it optionally displaces the targetTag before or after the inserted tag

Also, make defaults sensible

lukebarnard1 · 2017-12-06T14:55:16Z

This has sprouted a riot-web PR - element-hq/element-web#5790

Becuase the tests rely on being able to inspect the state of MatrixChat

React DnD specifies functions with upper-case first letters

dbkr · 2017-12-06T16:24:35Z

src/components/views/elements/DNDTagTile.js

+            tag: monitor.getItem().tag,
+            targetTag: props.groupProfile.groupId,
+            // Note: we indicate that the tag should be after the target when
+            // it's being dragged over the top half of the target.


bottom half, no?

Actually no, it really is being dragged over the top half. I just found this snappier.

oh, so it is - so after == above?

Nope, after == below. It feels snappier when you move a tile over the top half of another tile and the target is replaced with the one you're dragging, i.e. it's placed after the target.

dbkr · 2017-12-06T16:51:32Z

src/stores/TagOrderStore.js

+            // Initialise the state such that if account data is unset, default to the existing ordering
+            case 'all_tags':
+                this._setState({
+                    allTags: payload.tags.sort(), // Sort lexically


I don't think this will do the right thing when you join a new group: the new group will be added to allTags but won't propagate to orderedTags which is what the TagPanel reads, so they new group won't show up? Somewhere we'll have to work out what changed in the new set of tags and make the equivalent change to the orderedTags, preserving the order.

I assume the idea here is that the TagOrderStore knows nothing about groups, but it doesn't seem great that it's the responsibility of a component to give the store the set of all tags, ie. that component has to be mounted once (and only once?) in the view for the store to work. I'd guess the pattern I'd expect here would be to have another store or something for the complete set of tags, then the TagOrderStore consumes that and exposes the same info but ordered.

it's the responsibility of a component to give the store the set of all tags

I could move the fetch to get the list of groups to the TagOrderStore. The issue is that this would be perfect in an action creator, something that we haven't done before. Perhaps we should go and make a bunch of action creators and formalises all of our existing dispatches by wrapping them in functions (which is standard in other apps AFAICT).

have another store or something for the complete set of tags, then the TagOrderStore consumes that

Stores really should not communicate. all_tags is already being consumed by another store, and I believe the general pattern is to duplicate state as required such that the state of the store really only depends on its own previous state and the actions it receives.

I don't think this will do the right thing when you join a new group

This is true. If only our store could listen for the group membership update...

Having TagOrderStore fetch the list of groups is fine by me

dbkr · 2017-12-06T16:53:58Z

src/components/structures/LoggedInView.js

@@ -344,3 +346,5 @@ export default React.createClass({
        );
    },
 });
+
+export default DragDropContext(HTML5Backend)(LoggedInView);


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?)

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.

These can be used to dispatch actions immediately, or after some asynchronous work has been done. Also, create GroupActions.fetchJoinedGroups as an example. The concept of async action creators can be used in the following cases: - stores or views that do async work, dispatching based on the results - actions that have complicated payloads, would make more sense as functions with documentation that dispatch created actions.

lukebarnard1 · 2017-12-07T17:11:52Z

We agreed that I shall:

Move the group fetching logic to an action creator that will dispatch all_tags at some point. (Or probably joined_groups tbh).
Move the getAccountData in TagOrderStore to an action creator that is invoked whenever we get an account data event.
Move commit_tags to an action creator too

This introduces a generic way to register certain events emitted by the js-sdk as those that should be propagated through as dispatched actions. This allows the store to treat the js-sdk as the "Server" in the Flux data flow model. It also allows for stores to not be aware specifically of the matrix client if they are only reading from it.

lukebarnard1 · 2017-12-13T11:11:27Z

I'm a little uncertain as to whether commitTagOrdering and fetchJoinedGroups actually belong in the separate actions directory. They feel like they could just go in the same file as the views that use them (TagPanel, in this case). wdyt?

Separating actions from views is mostly separation of concerns (we don't want our views doing async requests) but it's also nice to be able to use them from anywhere (without refactoring them later).

richvdh · 2017-12-13T14:21:54Z

src/actions/GroupActions.js

@@ -18,6 +18,14 @@ import { asyncAction } from './actionCreators';

 const GroupActions = {};

+/**
+ * Create a GroupActions.fetchJoinedGroups action that represents an
+ * asyncronous request to fetch the groups to which a user is joined.


asynchronous

richvdh · 2017-12-13T14:24:47Z

src/actions/GroupActions.js

@@ -18,6 +18,14 @@ import { asyncAction } from './actionCreators';

 const GroupActions = {};

+/**
+ * Create a GroupActions.fetchJoinedGroups action that represents an


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.

richvdh · 2017-12-13T14:26:45Z