-
-
Notifications
You must be signed in to change notification settings - Fork 832
Conversation
lukebarnard1
commented
Sep 20, 2017
•
edited
Loading
edited
- Add "Add a User/Room" buttons and always display default lists
- Implement UserPickerDialog for adding users
- Add users to group summary using new API
- Implement avatar, displayname for featured users
- Disable "Add a Room" button for when we have a room picker
Also, use AccessibleButtons.
Profile data has been added to the API response for users in the group summary
if (!success) return; | ||
addrs.map((addr) => { | ||
return MatrixClientPeg.get() | ||
.addUserToGroupSummary(this.props.groupId, addr.address); |
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.
You're using map
here and returning the promise, but then discarding the result - if you're not interested in the result, forEach
may be better suited. That said, we should probably display an error if any of them failed, so we should probably not be ignoring the results (https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/GroupInvite.js#L43 is where I did this fwiw)
// Add avatar once we get profile info inline in the summary response | ||
//const BaseAvatar = sdk.getComponent("avatars.BaseAvatar"); | ||
const BaseAvatar = sdk.getComponent("avatars.BaseAvatar"); | ||
const name = this.props.summaryInfo.displayname || this.props.summaryInfo.user_id.slice(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.
why the slice(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.
Ah. I hadn't noticed that _getInitialLetter
ignores the "@".
this.setState({ | ||
busy: false, | ||
}); | ||
}); |
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.
Shouldn't this be fetching the group user list once and then doing a client-side filter when the query changes rather than waiting until you type something and then displaying the whole group user list?
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 would argue that it's best to get the group list frequently and avoid early optimisation (so that the user doesn't have to close and open the dialog once someone has accepted the invite to a group, for example). But yes. I totally forgot to actually implement the filtering. 🤦♂️