Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Allow user to choose from existing DMs on new chat#736

Merged
lukebarnard1 merged 6 commits into
developfrom
luke/chat-create-or-reuse-dialog
Mar 7, 2017
Merged

Allow user to choose from existing DMs on new chat#736
lukebarnard1 merged 6 commits into
developfrom
luke/chat-create-or-reuse-dialog

Conversation

@lukebarnard1
Copy link
Copy Markdown
Contributor

When creating a new chat with one person, show a dialog that asks the user whether they'd like to use an existing chat or actually create a new room.

Fixes element-hq/element-web#2760

When creating a new chat with one person, show a dialog that asks the user whether they'd like to use an existing chat or actually create a new room.

Fixes element-hq/element-web#2760
Copy link
Copy Markdown
Member

@dbkr dbkr 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

unread={Unread.doesRoomHaveUnreadMessages(room)}
highlight={highlight}
isInvite={me.membership == "invite"}
onClick={() => this.props.onFinished(true)}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is generally considered bad since a new arrow function will be created on each render, resulting in the props not being equal, which causes a DOM update on every render (it's equivalent to binding in render (https://ryanfunduk.com/articles/never-bind-in-render/).

In this case it's easy to work around since you can just have a member function that calls onFinished(true)


export default class CreateOrReuseChatDialog extends React.Component {

constructor (props) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Style nitpick: no need for space between method name & opening bracket.

@dbkr dbkr assigned lukebarnard1 and unassigned dbkr Mar 6, 2017
});
if (this.props.onClick) {
this.props.onClick();
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Presumably this is no longer necessary?

_onAction(payload) {
switch(payload.action) {
case 'view_room':
this.props.onFinished(true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Similar to the discussion in #riot-dev, I don't particularly like using view-room dispatches for this: I know there's a modal dialog being displayed at the time, but seems entirely possible that something else will decide to view a different room while the modal is being displayed, at which point the modal goes away. In essence, the thing that causes the modal to go away should be the user interaction with the dialog finishing, not the currently viewed room changing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

seems entirely possible that something else will decide to view a different room while the modal is being displayed

This is true. I would feel better about using a callback if that was the only thing that RoomTile was responsible for. i.e. move view_room dispatch out of RoomTile and into the onClick that gets passed into it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I was going to say it's not great that something ekse also happens in addition to the onClick, but didn't really want to make you change all the other places RoomTile is used. Up to you. :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Refactoring FTW!

Luke Barnard added 2 commits March 6, 2017 17:44
But make sure that nothing other than the callback is done when RoomTile is clicked.
@lukebarnard1
Copy link
Copy Markdown
Contributor Author

Requires element-hq/element-web#3376, which should be merged first.

@lukebarnard1 lukebarnard1 merged commit b7f1b1a into develop Mar 7, 2017
martindale pushed a commit to FabricLabs/matrix-react-sdk that referenced this pull request Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants