-
Notifications
You must be signed in to change notification settings - Fork 97
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
Nick/websockets #69
Nick/websockets #69
Conversation
src/modules/Chat/Chat.tsx
Outdated
); | ||
return !isEmpty(username) ? ( | ||
<> | ||
{chat.map(({ author, message }, idx) => ( |
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.
может назовем как-то по-другому? можно и в store, ну или хотя бы при импорте
потому что я бы не догадался, что это список
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.
Плиз, накинь, как стоит переименовать?) Не придумал ничего (((
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.
Придумалось только "message 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.
chatMessages?
Я вот тоже не могу придумать особо хороших вариантов, так что можно считать коммент не очень валидным (таки критикую предлагай)
src/modules/Chat/Chat.tsx
Outdated
); | ||
return !isEmpty(username) ? ( | ||
<> | ||
{chat.map(({ author, message }, idx) => ( |
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.
ну и я бы все же id добавил к сообщениям (хотя бы синтетические, на клиентской стороне)
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.
А можешь плиз подсказать, что тут может пойти не так?
key={
${author}${message}${idx}}
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.
ну мало ли - у тебя добавится в середину такой же коммент, я про общее правило про индексы в виде ключей
у меня нет конкретного кейса, когда это плохо
src/modules/Chat/Chat.tsx
Outdated
<input | ||
placeholder="Enter your msg" | ||
value={message} | ||
onChange={(ev) => setMsg((ev.target as HTMLInputElement).value)} |
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.
а для сабмита useCallback =))
…-tutorial into nick/websockets
message: string; | ||
}; | ||
|
||
export class ChatComponent extends Component<Props, State> { |
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.
@vvscode я переписал этот компонент на класс, смотрел на него некоторое время, так и не смог понять, в чем выйгрыш версии с хуками(((
За исключением того, что хуки это не JS и надо понимать, во что это превращается, что мне тяжело (это о недостатках)
5ec9442
to
8f9c1bc
Compare
No description provided.