-
-
Notifications
You must be signed in to change notification settings - Fork 833
Refactor ChatInviteDialog to be UserPickerDialog #1300
Conversation
Now it's just a means of choosing users and all the actual inviting functionality is moved out to Invite.js. This will allow us to reuse it for inviting to groups. Adds the ability to restrict what types of addresses may be chosen, although this isn;t used yet, it will be necessary for groups because groups don't support 3pid invites.
The other changes I forgot to add
src/Invite.js
Outdated
import MatrixClientPeg from './MatrixClientPeg'; | ||
import MultiInviter from './utils/MultiInviter'; | ||
import Modal from "./Modal"; |
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.
nit: consistent quotes pls
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.
done
src/Invite.js
Outdated
import MatrixClientPeg from './MatrixClientPeg'; | ||
import MultiInviter from './utils/MultiInviter'; | ||
import Modal from "./Modal"; | ||
import createRoom from './createRoom'; | ||
import sdk from "./"; |
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.
and here
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.
done
React.PropTypes.element, | ||
React.PropTypes.string, | ||
title: PropTypes.string.isRequired, | ||
description: PropTypes.oneOfType([ |
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.
could use PropTypes.node
I think
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.
done
}; | ||
}, | ||
|
||
getInitialState: function() { | ||
return { | ||
error: false, | ||
|
||
// List of AddressTile.InviteAddressType objects representing | ||
// List of InviteAddressType objects representing |
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.
It's a bit odd having the word invite
in variables, comments now that this component doesn't know about inviting
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.
have made it UserAddressType throughout
@@ -311,7 +268,7 @@ module.exports = React.createClass({ | |||
// This is important, otherwise there's no way to invite | |||
// a perfectly valid address if there are close matches. | |||
const addrType = getAddressType(query); | |||
if (addrType !== null) { | |||
if (this.props.validAddressTypes.indexOf(addrType) !== -1) { |
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.
Could use includes
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've been sticking to indexOf, but probably for no reason really, so I don't care, done
@@ -90,50 +89,7 @@ module.exports = React.createClass({ | |||
inviteList = this._addInputToList(); |
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.
There are still things called invite
and comments mentioning invites in this file
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.
done
src/Invite.js
Outdated
// could be a third party identifier or a matrix ID) | ||
// along with some additional information about the | ||
// address / target. | ||
export const UserAddressType = PropTypes.shape({ |
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 feels like a strange place for a general "user address" type that might no have anything to do with inviting. Putting it in AddressTile
seems fine?
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.
Moved to UserAddress as otherwise we'd end up importing view code from utility code which is definitely a pattern I'd like to avoid.
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.
LGTM :)
I managed to lose this when refactoring ChatInviteDialog in #1300 Fixes element-hq/element-web#5119
Now it's just a means of choosing users and all the actual inviting
functionality is moved out to Invite.js. This will allow us to
reuse it for inviting to groups.
Adds the ability to restrict what types of addresses may be chosen,
although this isn't used yet, it will be necessary for groups
because groups don't support 3pid invites.