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

Direct message chat #334

Merged
merged 9 commits into from
Jan 25, 2023
Merged

Direct message chat #334

merged 9 commits into from
Jan 25, 2023

Conversation

mrval5
Copy link
Contributor

@mrval5 mrval5 commented Jan 24, 2023

What does this do?

Direct messages
Task: https://wilderworld.atlassian.net/browse/ZOS-69
Demo: https://www.loom.com/share/9ba37998cc324e78a34b20d805b4582d

Why are we making this change?

How do I test this?

Key decisions and Risk Assessment:

Things to consider:

  1. How will this affect security?
  2. How will this affect performance?
  3. Does this change any APIs?

@mrval5 mrval5 requested a review from a team January 24, 2023 09:34
AdamDotCom
AdamDotCom previously approved these changes Jan 24, 2023
Copy link
Contributor

@AdamDotCom AdamDotCom left a comment

Choose a reason for hiding this comment

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

Looks great. How hard would it be to add autofocus to the DM message input when the DM window is opened?

<IconButton
className='sidekick__tabs-network'
icon={Icons.Network}
onClick={this.clickTab.bind(this, Tabs.NETWORK)}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we changed our clickTab method above can we get rid of the bind?

 clickTab = (tab: Tabs) =>
onClick={this.clickTab(Tabs.NETWORK)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I remove the .bind and keep onClick={this.clickTab(Tabs.NETWORK)} it will call the method on each render and to solve it I have to do onClick={() => this.clickTab(Tabs.NETWORK)} which it will create an anonymous function on each render which do not want.

Copy link
Contributor

@dalefukami dalefukami Jan 30, 2023

Choose a reason for hiding this comment

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

...it will create an anonymous function on each render which do not want.

Just as an FYI, bind is creating an entirely new function on every render too. So, using bind isn't really a solution to the problem you've mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, I was wrong about it, it's also written in react documentation.

@dalefukami for learning purposes..., do you have an idea how we can make it better in performance ?

Copy link
Contributor

@dalefukami dalefukami Jan 31, 2023

Choose a reason for hiding this comment

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

There are a couple of strategies that I tend towards:

  1. If the list is fairly short and static (like this case) I'll just write 3 separate functions on the class. this.clickNetworkTab: () => this.clickTab(Tabs.NETWORK). They can all call the general case and they're super short so it's pretty obvious that they're just shortcut methods for this purpose.
  2. If the list is dynamic then the main strategy is to cache the functions as they're created the first time. I saw this used in zero-web and it's pretty similar to what I've done previously.
  3. If the child component is a specific thing that is always supposed to render something of a "type" or "id"...then I'll usually make the child component call the onClick with the appropriate value. In this case, it probably doesn't make sense to use this strategy.

return (
<div
className='direct-message-members__user'
onClick={this.handleMemberClick.bind(this, directMessage.id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Another bind

src/store/direct-messages/saga.test.ts Outdated Show resolved Hide resolved
src/store/direct-messages/api.ts Outdated Show resolved Hide resolved
@mrval5
Copy link
Contributor Author

mrval5 commented Jan 25, 2023

Looks great. How hard would it be to add autofocus to the DM message input when the DM window is opened?

not sure yet, let's make a task for it

@mrval5 mrval5 merged commit 60a9e48 into main Jan 25, 2023
@mrval5 mrval5 deleted the direct-message-chat branch January 25, 2023 10:23
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

4 participants