-
-
Notifications
You must be signed in to change notification settings - Fork 833
Conversation
Somewhat untested, sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First review as discussed, 1 question about the "hard" limit error, otherwise looks good. Build is broken but you mentioned this needs more work.
}, | ||
|
||
componentWillUnmount: function() { | ||
document.removeEventListener('keydown', this._onKeyDown); | ||
this._matrixClient.removeListener("accountData", this.onAccountData); | ||
this._matrixClient.removeListener("sync", this.onSync); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uff, good catch
if (this.props.kind === 'hard') { | ||
toolbarClasses['mx_MatrixToolbar_error'] = true; | ||
content = _t( | ||
"This homeserver has hit its Monthly Active User limit. Please <a>contact your service administrator</a> to continue using the service.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be shown when (initial) sync has failed I think, but wouldn't it be very confusing to just show a banner, but still the complete UI, but you can't interact with the server in any meaningful way? (or is that not what M_MAU_LIMIT_EXCEEDED causes?). Maybe I'm missing something, but cancelling login and showing an error message on the login page would be clearer if this is the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is that you can still see the messages you've already synced, although it's tied quite heavily into the fact that causing a logout causes the client to delete its e2e keys: I think this may be the intention in the long term once we work out how to do it.
The initialisation here is done on a timer to allow the DOM elements to be in place before it's run, so it's possible for an update to happen ebfore the initialisation. Stop & bail out if there is no this.stickies.
Somewhat untested, sorry