Truncate consecutive member events #544

lukebarnard1 · 2016-11-08T16:37:13Z

This is needed for the IRC bridge to be able to do full membership list syncing (with quit/netsplit debouncing) without cluttering the message panel.

(See element-hq/element-web#349)

Also, for CSS changes, see element-hq/element-web#2565

This is needed for the IRC bridge to be able to do full membership list syncing without cluttering the message panel.

matrixbot · 2016-11-08T16:37:15Z

Can one of the admins verify this patch?

lukebarnard1 · 2016-11-08T17:18:15Z

Note to self: test for element-hq/element-web#2020

@richvdh

netsplits across midnight is not handled, and @richvdh suggested splitting the list in two

Now it's: 1 user joined and left >1 others joined and left

lukebarnard1 · 2016-11-10T11:01:57Z

Things look a bit tidier now, with a summary followed by collapsed events:

A IRCAS bridged netsplit (/w incremental syncing turned on, QUIT debouncing off) where one user does not return following netsplit would look like this:

richvdh

generally this looks like some great stuff! a few comments though :)

richvdh · 2016-11-10T11:10:13Z

src/components/structures/MessagePanel.js

@@ -20,6 +20,7 @@ var dis = require("../../dispatcher");
 var sdk = require('../../index');

 var MatrixClientPeg = require('../../MatrixClientPeg')
+var MemberEventListSummary = require('../views/elements/MemberEventListSummary.js');


standard way to do this is with

const MemberEventListSummary = sdk.getComponent('views.elements.MemberEventListSummary');

in the relevant render function.

@richvdh how does one make a component exist when using getComponent?

richvdh · 2016-11-10T11:11:37Z

src/components/structures/MessagePanel.js

@@ -286,6 +287,47 @@ module.exports = React.createClass({

            var last = (i == lastShownEventIndex);

+            var isMembershipChange = (e) =>


suggest hoisting this out of the for loop so that it doesn't need to be redefined for each event

richvdh · 2016-11-10T11:14:28Z

src/components/structures/MessagePanel.js

+                    }
+                    summarisedEvents.push(collapsedMxEv);
+                }
+                let renderEvents = (pEvent, events) => {


let's have a comment here to say what we expect i to be. Is it the last in the group, or the first that's not in the group?

richvdh · 2016-11-10T11:32:38Z

src/components/structures/MessagePanel.js

+                    <MemberEventListSummary
+                        events={summarisedEvents}
+                        previousEvent={prevEvent}
+                        renderEvents={renderEvents}


passing in a render callback feels pretty icky here.

An alternative might be to hoist the event filtering logic in MemberEventListSummary.render to here, then generate the tiles here, and then set them as children of the MemberEventListSummary:

const filteredEvents = filterEvents(summarisedEvents); /* as per MemberEventListSummary.render() */ const tiles = filteredEvents.map( /* as above*/ ); <MemberEventListSummary events={summarisedEvents}> {tiles} </MemberEventListSummary>

and in MemberEventListSummary.render():

expandedEvents = this.props.children;

WDYT?

I did have that thought. Yep, on reflection, WFM.

richvdh · 2016-11-10T11:33:07Z

src/components/structures/MessagePanel.js

+                        renderEvents={renderEvents}
+                    />
+                );
+                prevEvent = mxEv;


don't you want the last event in the group, rather than the first?

Yep, fair enough!

richvdh · 2016-11-10T11:50:38Z

src/components/views/elements/MemberEventListSummary.js

+
+    renderAvatars: function(events) {
+        let avatars = events.slice(0, this.props.avatarsMaxLength).map((e) => {
+            let onClickAvatar = () => {


I'd be sorely tempted to push this down into MemberAvatar, maybe with a viewUserOnClick=true property, as a way to factor out the common code with MessagePanel.

richvdh · 2016-11-10T11:51:59Z

src/components/views/elements/MemberEventListSummary.js

+    render: function() {
+        let summary = null;
+
+        // Reorder events so that joins come before leaves


as above - suggest moving this logic to timelinepanel.

How much are we talking? I can't move filteredEvents on it's own because of the calculation for joinAndLeft

oh, I think I misread before: I thought that the list being passed into renderEvents depended on the filtering calculations. Looks like it doesn't, so this is fine.

richvdh · 2016-11-10T11:54:36Z

src/components/views/elements/MemberEventListSummary.js

+            toggleButton = (
+                <a onClick={this.toggleSummary} href="javascript:;">{expanded?'collapse':'expand'}</a>
+            );
+            let noun = (joinAndLeft === 1 ? 'user' : 'others');


so... if 2 users join and leave, and nobody joins without leaving or vice versa... we'll just see "2 others joined and left"?

Fair enough, I'll update that to be either 'others' or 'users' as appropriate.

richvdh · 2016-11-10T11:54:57Z

src/components/views/elements/MemberEventListSummary.js

+        if (!fewEvents) {
+            summary = this.renderSummary(joinEvents, leaveEvents);
+            toggleButton = (
+                <a onClick={this.toggleSummary} href="javascript:;">{expanded?'collapse':'expand'}</a>


don't think you need an href here.

richvdh · 2016-11-10T11:59:58Z

src/components/views/elements/TruncatedList.js

@@ -28,17 +28,29 @@ module.exports = React.createClass({
        createOverflowElement: React.PropTypes.func
    },

+    getInitialState: function() {


this looks unrelated... has it got lost?

I'm not quite sure what the intention is with it - it looks like what you are doing would be better handled by setting truncateAt = -1

oops, didn't mean to commit this to the PR; I shall remove

lukebarnard1 · 2016-11-10T15:53:02Z

@richvdh how does one make a component exist when using getComponent?

richvdh · 2016-11-10T16:47:42Z

src/components/structures/MessagePanel.js

+                    }
+                    summarisedEvents.push(collapsedMxEv);
+                }
+                // At this point, i = this.props.events.length OR i = the index of the first event


are you sure? line 303 suggests otherwise

ah yes, good shout!

richvdh · 2016-11-10T16:48:43Z

src/components/structures/MessagePanel.js

+                    ).reduce((a,b) => a.concat(b));
+                };
+
+                let eventTiles = renderEvents(prevEvent, summarisedEvents);


inline renderEvents?

richvdh · 2016-11-10T16:49:12Z

src/components/views/avatars/MemberAvatar.js

-        resizeMethod: React.PropTypes.string
+        resizeMethod: React.PropTypes.string,
+        // Whether the onClick of the avatar should dispatch 'view_user'
+        viewUserOnClick: React.PropTypes.boolean


nit: trailing comma would be nice

richvdh · 2016-11-10T16:51:56Z

src/components/views/avatars/MemberAvatar.js

    },

    getDefaultProps: function() {
        return {
            width: 40,
            height: 40,
-            resizeMethod: 'crop'
+            resizeMethod: 'crop',
+            viewUserOnClick: false


richvdh · 2016-11-10T16:53:16Z

src/components/views/avatars/MemberAvatar.js

@@ -65,9 +69,19 @@ module.exports = React.createClass({

        var {member, ...otherProps} = this.props;

+        var onClick = null;


some uses of MemberAvatar set props.onClick to other things; you'll need to pass that down as the default, and probably exclude it from otherProps above.

richvdh · 2016-11-10T16:58:22Z

src/components/views/elements/MemberEventListSummary.js

+    render: function() {
+        let summary = null;
+
+        // Reorder events so that joins come before leaves


oh, I think I misread before: I thought that the list being passed into renderEvents depended on the filtering calculations. Looks like it doesn't, so this is fine.

richvdh · 2016-11-10T17:09:08Z

src/components/views/elements/MemberEventListSummary.js

+
+        // Special case the last name. ' and ' might be included later
+        // So you have two cases:
+        if (originalNumber <= this.props.summaryLength) {


this is a bit tortuous now. How about:

let lastName = this._getEventSenderName(lastEvent); if (names.length === 0) { // special-case for a single event return lastName; } let remaining = originalNumber - this.props.summaryLength; if (remaining > 0) { // name1, name2, name3 and 100 others return names + ', ' + lastName + ', and ' + remaining + ' others'; } else { // name1, name2 and name3 return names + ' and ' + lastName; }

richvdh · 2016-11-10T17:12:16Z

matrixbot: ok to test

matrix-org/matrix-react-sdk#544

richvdh

lgtm otherwise

richvdh · 2016-11-10T17:58:52Z

src/components/views/elements/MemberEventListSummary.js

-            // name1, name2, name3 [and 100 others]
-            names += ', ';
+        let lastName = this._getEventSenderName(lastEvent);
+        if (names.length === 0) {


this makes lines 74-76 redundant

richvdh · 2016-11-11T11:01:44Z

lgtm!

Truncate consecutive member events

cd241a4

This is needed for the IRC bridge to be able to do full membership list syncing without cluttering the message panel.

Luke Barnard added 4 commits November 9, 2016 16:03

WIP nicer join/part folding

2f02c4b

Trying to get rid of the date separator problem

04a934f

Add date seperator to the expanded events

5f0b891

netsplits across midnight is not handled, and @richvdh suggested splitting the list in two

Removed logs, updated phrasing on 'others joined'

f5bc8d1

Now it's: 1 user joined and left >1 others joined and left

Remove empty lines

79c01fa

lukebarnard1 assigned richvdh Nov 10, 2016

richvdh suggested changes Nov 10, 2016

View reviewed changes

Luke Barnard added 14 commits November 10, 2016 13:01

Use sdk.getComponent

d01bb0b

Hoist isMembershipChange out of for loop

0ec5eeb

Comment on i after membershipchanges

764959e

Render children EventTiles before passing to summary element

566034c

Unnecessary props

e293637

Prefer import React, set prevEvent to last MemberhsipChange

dcbb312

events are required

8422017

Use _ prefix for private methods

370f5f0

Children are required

d6b74be

Factor out "and others"

6b2aa43

Factor out avatar click leads to view_user

d8bc4d9

remove href

1a4df38

Prefer null to falsy, use correct plural

f71ac93

revert now redundant TruncatedList edits

ac46012

richvdh reviewed Nov 10, 2016

View reviewed changes

Luke Barnard added 2 commits November 10, 2016 17:03

npm run reskindex

e05aeb3

Add class to expand link for pointer and colour

b11dd95

richvdh suggested changes Nov 10, 2016

View reviewed changes

richvdh assigned lukebarnard1 and unassigned richvdh Nov 10, 2016

lukebarnard1 pushed a commit to element-hq/element-web that referenced this pull request Nov 10, 2016

Add CSS for mlist truncation

4f944cf

matrix-org/matrix-react-sdk#544

lukebarnard1 mentioned this pull request Nov 10, 2016

Add CSS for mlist truncation element-hq/element-web#2565

Merged

Luke Barnard added 5 commits November 10, 2016 17:22

correct comment

d2d7ebc

Inline renderEvents

3ec7940

trailing commas

87e8f0e

pass in prop onClick by default

0695037

Refactor renderNameList (thanks, @richvdh)

3d7f691

richvdh approved these changes Nov 10, 2016

View reviewed changes

Remove redundant lines

d3a5487

richvdh merged commit 00ecff7 into develop Nov 11, 2016

This was referenced Nov 11, 2016

Fix MemberAvatar PropTypes & MemberEventListSummary key #547

Merged

Too many consecutive invisible events in the timeline crash the browser element-hq/element-web#2020

Closed

		@@ -286,6 +287,47 @@ module.exports = React.createClass({

		var last = (i == lastShownEventIndex);

		var isMembershipChange = (e) =>

		@@ -65,9 +69,19 @@ module.exports = React.createClass({

		var {member, ...otherProps} = this.props;

		var onClick = null;

Truncate consecutive member events #544

Truncate consecutive member events #544

Conversation

lukebarnard1 commented Nov 8, 2016 • edited Loading

matrixbot commented Nov 8, 2016

lukebarnard1 commented Nov 8, 2016

lukebarnard1 commented Nov 10, 2016

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukebarnard1 commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Nov 10, 2016

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Nov 11, 2016

lukebarnard1 commented Nov 8, 2016 •

edited

Loading

lukebarnard1 Nov 10, 2016 •

edited

Loading

richvdh Nov 10, 2016 •

edited

Loading