Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make RoomTooltip generic and add ContextMenu&Tooltip to GroupInviteTile #1950

Merged
merged 5 commits into from
Jun 14, 2018

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jun 12, 2018

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Copy link
Member Author

t3chguy commented Jun 12, 2018

Could not have on same branch as dep as there is another PR depending on the same js-sdk PR

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@lukebarnard1 lukebarnard1 self-assigned this Jun 13, 2018
Copy link
Contributor

@lukebarnard1 lukebarnard1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM 👍

description: _t("Unable to reject invite"),
});
}
modal.close();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in a finally clause otherwise we'll get stuck with a spinner if an error is thrown in the catch.

group: this.props.group,
onFinished: () => {
this.setState({ menuDisplayed: false });
// this.props.refreshSubList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented code needs removal

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Copy link
Member Author

t3chguy commented Jun 14, 2018

@lukebarnard1 sorry added a bit more, filtering of the Group Invite Tiles

@lukebarnard1
Copy link
Contributor

Those changes look totally unrelated. I suggest making a new PR in future.

Copy link
Contributor

@lukebarnard1 lukebarnard1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise


const GroupInviteTile = sdk.getComponent('groups.GroupInviteTile');
for (const group of MatrixClientPeg.get().getGroups()) {
if (group.myMembership !== 'invite') continue;
ret.push(<GroupInviteTile key={group.groupId} group={group} collapsed={this.props.collapsed} />);
const {groupId: id, name, myMembership: membership} = group;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest keeping the variable names here; groupId and myMembership are fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed them when trying to squeeze it onto one line :P changed them back now :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah :) thanks

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@lukebarnard1 lukebarnard1 merged commit e81edd9 into develop Jun 14, 2018
@t3chguy t3chguy deleted the t3chguy/group_invite_contextmenu branch May 25, 2020 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants