Repository navigation
Conversation
This allows drafts to be stored per conversation instead of keeping the same draft for all conversations. However, it doesn't handle drafts in game chats the same way, because those conversations are not stored in the same way.
|
Hi Logan. Thanks for the PR! This is a good start. There are a few things to mention. First and biggest: the approach you took is to save the draft text into a conversation record after every change to the text input. While this is the simplest, most straight-forward approach, in practice doing it this way can bog down performance. Currently Shin uses a single store for the entire app, and when its state changes, the entire app re-renders from top to bottom. So after each keypress, the entire screen re-renders, not just the message bar. To fix this, you can use component-local state to store the latest value of the input, and do a rate-limited update to the global conversation state (maybe 'SAVE_CHAT_DRAFT' as a message type). If you wanted to get fancy, you could even create a reusable text input with that feature built-in (value changes as you type, and then runs a debounced callback every X milliseconds). Sidebar: in the future, we may have components subscribe to only the subset of state they need from the store, so app state changes don't trigger unnecessary render checks everywhere (we do use PureComponent which does not re-render unless props actually change). For most cases (other than inputs), a single top-down data flow is simpler. Other issues:
|
| type Props = { | ||
| conversation: ?Conversation, | ||
| onSubmit: string => any; | ||
| onDraft: string => any; |
There was a problem hiding this comment.
Given that this is not used for games, I'd make it optional: onDraft?: string => any
| } | ||
| } | ||
|
|
||
| _onChange(event: SyntheticEvent) { |
There was a problem hiding this comment.
The convention we're using in Shin is to use class property arrow functions for event handlers:
_onChange = (event: SyntheticEvent) => {
This convention sets this appropriately, removing the need for bind later on.
| type='text' | ||
| placeholder={placeholder} | ||
| value={draft === '' ? null : draft} | ||
| > |
There was a problem hiding this comment.
see prior comment about arrow functions for event handlers
| }} | ||
| /> | ||
| > | ||
| > |
There was a problem hiding this comment.
see previous comment about making onDraft optional, in which case this can be left out
|
Thanks for the thorough feedback! I'll start taking care of those things. At a first glance, it looks like storing the input text value in component local state and making |
|
Regarding debouncing -- do you have a preference for adding a dependency (e.g. lodash.debounce) vs sticking a debounce implementation in the utils? |
|
@loganmhb A debounce util function is preferred |
|
I think the latest commit finishes implementing your suggestions. Let me know if there's anything else you notice or would like changed. |
|
thanks, will review when I get a chance |
|
Almost there! A couple issues:
Code wise it's looking nice. 👍 |
|
@loganmhb Could you rebase this PR? |
First of all, thanks for a great-looking app! I'm really looking forward to seeing this grow. I did some work on #92 because it seemed like a nice-sized task to start learning my way around the codebase.
This pull request allows drafts to be stored per conversation instead of keeping the same draft for all conversations, resolving #92. However, it doesn't (yet?) handle drafts in game chats (it ignores them), because those conversations are not stored in the same way as far as I can tell. I'd be happy to work on improving that as well, but I figured I'd go ahead and put this up for feedback first.
Disclaimer: I'm a backend developer by day and I don't have much experience with React/Flow, so if there are rookie mistakes, obvious problems, or you want to take a different approach that feedback is more than welcome. I also haven't figured out how to test against an API server that's not KGS itself, so I haven't tested submission of chat messages.