Skip to content

work in progress #15

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.

Sign up for GitHub

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

Draft
lx93 wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

work in progress #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8599f1
work in progress
lx93 Aug 19, 2019
9573e94
refactored the test file, but still failing one test. please review.
lx93 Oct 12, 2019
9a569d8
refactored the test file, but still failing one test. please review.
lx93 Oct 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,8 @@
"devDependencies": {
Copy link
Owner

@imann24 imann24 Oct 12, 2019

Choose a reason for hiding this comment

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

Having some functional issues when I test the app:

  1. Not able to send messages with this version
  2. When I start the chat there are empty chat bubbles
  3. Think it would preferable to aim for the flow that we create a new chat bubble when the other person starts speaking

"concurrently": "^4.0.1",
"recursive-install": "^1.4.0"
},
"dependencies": {
"react-chat-ui": "^0.3.2"
Copy link
Owner

@imann24 imann24 Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The hierarchy of this repo is kind of difficult. So:

|-package.json : top level development dependencies

|-- src
|--- server
|---- package.json : dependencies for frontend (probably where this belongs)

|-- src
|--- server
|---- package.json : dependencies for backend/Express server

Copy link
Collaborator Author

@lx93 lx93 Aug 20, 2019

Choose a reason for hiding this comment

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

fixed!

Copy link
Owner

@imann24 imann24 Oct 12, 2019

Choose a reason for hiding this comment

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

You may need to run npm unstainll react-chat-ui at the top level directory to remove this change (or just deletethese lines from the JSON)

}
}
47 changes: 29 additions & 18 deletions src/client/src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import React, { Component } from 'react';
import logo from './logo.svg';
import './App.css';
import Socket from './socket'
import {Conversation, Message, Sender} from './message'
import {Conversation, Msg, Sender} from './message'
import { ChatFeed, Message } from 'react-chat-ui';


class App extends Component {
socket = new Socket(document.location.pathname.replace('/', ''))
Expand All @@ -22,23 +24,25 @@ class App extends Component {
}
}
onKeyPress = event => {
if (event.key === ' ') {
if (this.state.text === ' ') {
this.setState({text: ''});
} else {
this.handleSubmit();
}
}
// if (event.key === ' ') {
Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

Is the plan to re-add the commented out code?

Copy link
Collaborator Author

@lx93 lx93 Aug 20, 2019

Choose a reason for hiding this comment

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

we need to talk about the userflow and UX, as mentioned below. do we want each word occupy one bubble, and as user type each word, more bubbles will pop out?

Copy link
Owner

@imann24 imann24 Aug 21, 2019

Choose a reason for hiding this comment

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

Does this answer: https://github.com/imann24/now/pull/15/files#r315968692 ?

Copy link
Owner

@imann24 imann24 Oct 12, 2019

Choose a reason for hiding this comment

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

Planning to remove this code?

// if (this.state.text === ' ') {
// this.setState({text: ''});
// } else {
// this.handleSubmit();
// }
// }
this.handleSubmit();
Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

May want to re-consider this spec in future? Sending a message through the socket on every letter typed may cause some issues (too much network traffic)

Copy link
Collaborator Author

@lx93 lx93 Aug 20, 2019 •
edited by imann24
Loading

Choose a reason for hiding this comment

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

okay, whats your thoughts on this? My impression was that, as was described, user can see each letters being typed/deleted in real-time. also, i found that instead of using onKeyPress props we could use onKeyDown to trigger handleSubmit() even while user pressed backspace/shift/ctrl key.

Copy link
Collaborator Author

@lx93 lx93 Aug 20, 2019

Choose a reason for hiding this comment

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

regarding network throughput, is there anyway we could make that work? how much traffic could websocket handle?

Copy link
Owner

@imann24 imann24 Aug 21, 2019

Choose a reason for hiding this comment

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

Yeah, I like realtime if it's possible. Would probably have to load test to know. Want to write us some automated tests? 😅

Let's keep it live for now. We can switch it back to sending every word if we run into performance issues

}
handleSubmit = async e => {
if (e) {
e.preventDefault();
}
let newMessage = new Message(this.state.user, this.state.text)
this.setState(previousState => ({
Copy link
Owner

@imann24 imann24 Oct 12, 2019

Choose a reason for hiding this comment

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

Believe we still need a call to update the React state

Copy link
Collaborator Author

@lx93 lx93 Oct 15, 2019

Choose a reason for hiding this comment

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

which state should we update in handleSubmit?

Copy link
Owner

@imann24 imann24 Oct 16, 2019

Choose a reason for hiding this comment

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

this.state.conversation and this.state.text

text: "",
conversation: previousState.conversation.addMessage(newMessage),
}));
// alert(this.state.text)
let newMessage = new Msg(this.state.user, this.state.text)
// this.setState(previousState => ({
// text: "",
// conversation: previousState.conversation.addMessage(newMessage),
// }));
this.socket.sendMessage(newMessage);
};
render() {
Expand All @@ -47,6 +51,8 @@ class App extends Component {
<header className="App-header">
<img src={logo} className="App-logo" alt="logo" />
</header>


Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

Nice spacing for visual clarity!

<form onSubmit={this.handleSubmit}>
<p>
<strong>Say Something, {this.state.user.name}:</strong>
Expand All @@ -55,12 +61,17 @@ class App extends Component {
type="text"
value={this.state.text}
onKeyPress={this.onKeyPress}
onChange={e => this.setState({ text: e.target.value })}/>
onChange={e => this.setState({ text: e.target.value },()=> console.log())}/>
Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

Empty console log?

Copy link
Collaborator Author

@lx93 lx93 Aug 20, 2019

Choose a reason for hiding this comment

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

lol fixed

Copy link
Owner

@imann24 imann24 Oct 12, 2019

Choose a reason for hiding this comment

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

Perhaps some of your changes aren't committed yet?

</form>
{this.state.conversation.messages.slice().reverse().map((value, index) => {
return <p key={index}><b>{value.sender.name}: </b>{value.text}</p>
})}
</div>

<ChatFeed
Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

Very clean integration with the React component! Finally make React work for us!

messages={[new Message({id: 1,message: this.state.conversation.lastMessage.text,senderName:this.state.user.name}),
Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

Hmm, so looks like this kind of changes the spec.

I would imagine that breaks in conversation create a new chat bubble. Instead of only have a single chat bubble per user

Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

Also we should clarify if a chat session only allows 2 users max

Copy link
Collaborator Author

@lx93 lx93 Aug 20, 2019

Choose a reason for hiding this comment

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

what if we only allow 2 users for now, for simplicity sake. and we can add group chat functionality in later version.

Copy link
Collaborator Author

@lx93 lx93 Aug 20, 2019

Choose a reason for hiding this comment

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

Hmm, so looks like this kind of changes the spec.

I would imagine that breaks in conversation create a new chat bubble. Instead of only have a single chat bubble per user

do you mean each word would occupy one bubble, and as user type each word, more bubbles will pop out?

Copy link
Owner

@imann24 imann24 Aug 21, 2019

Choose a reason for hiding this comment

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

Sounds good on 2 users. Spun up an extra ticket to cover error handling (e.g. a 3rd user tries to join): #17

Let's see Issue #17 is outside the scope of this PR though and tackle that later.

In terms of bubbles, my thought is: create new bubbles whenever the person talking changes. For example:

Isaiah is talking (bubble 1 - blue)
Bill starts talking (bubble 2 - white)
Isaiah resumes talking (bubble 3 - blue)
Bill resumes talking (bubble 4 - white)

What do you think?

Copy link
Owner

@imann24 imann24 Aug 20, 2019

Choose a reason for hiding this comment

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

Oh! Now I see that we needed to rename Msg to avoid a conflict w/ the React component. Good call!

new Message({id: 0,message: this.state.text,senderName:this.state.user.name})]}
showSenderName
/>


</div>
);
}
}
Expand Down
18 changes: 9 additions & 9 deletions src/client/src/__tests__/message.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {Conversation, Message, Sender} from '../message'
import {Conversation, Msg, Sender} from '../message'

describe('Conversation tests', () => {
it('creates an empty array of messages', () => {
Expand All @@ -20,7 +20,7 @@ describe('Conversation tests', () => {
conversation.addMessage(message);

const receivedMessage = conversation.messages[0];
expect(receivedMessage).toBeInstanceOf(Message);
expect(receivedMessage).toBeInstanceOf(Msg);
expect(receivedMessage.sender).toBeInstanceOf(Sender);
expect(receivedMessage.text).toBe(message.text);
expect(receivedMessage.sender.name).toBe(message.sender.name);
Expand All @@ -47,7 +47,7 @@ describe('Conversation tests', () => {

const receivedMessage = conversation.messages[0];
expect(conversation.length).toBe(1);
expect(receivedMessage).toBeInstanceOf(Message);
expect(receivedMessage).toBeInstanceOf(Msg);
expect(receivedMessage.sender).toBeInstanceOf(Sender);
expect(receivedMessage.text).toBe(firstMessage.text + secondMessage.text);
expect(receivedMessage.sender.name).toBe(sender.name);
Expand Down Expand Up @@ -77,12 +77,12 @@ describe('Conversation tests', () => {
const receivedMessageSender1 = conversation.messages[0];
const receivedMessageSender2 = conversation.messages[1];
expect(conversation.length).toBe(2);
expect(receivedMessageSender1).toBeInstanceOf(Message);
expect(receivedMessageSender1).toBeInstanceOf(Msg);
expect(receivedMessageSender1.sender).toBeInstanceOf(Sender);
expect(receivedMessageSender1.text).toBe(messageSender1.text);
expect(receivedMessageSender1.sender.name).toBe(messageSender1.sender.name);
expect(receivedMessageSender1.sender.id).toBe(messageSender1.sender.id);
expect(receivedMessageSender2).toBeInstanceOf(Message);
expect(receivedMessageSender2).toBeInstanceOf(Msg);
expect(receivedMessageSender2.sender).toBeInstanceOf(Sender);
expect(receivedMessageSender2.text).toBe(messageSender2.text);
expect(receivedMessageSender2.sender.name).toBe(messageSender2.sender.name);
Expand Down Expand Up @@ -115,12 +115,12 @@ describe('Conversation tests', () => {
});
});

describe('Message tests', () => {
describe('Msg tests', () => {
it('initializes correctly', () => {
const sender = new Sender('NAME');
const text = 'TEXT';

const message = new Message(sender, text);
const message = new Msg(sender, text);

expect(message.sender).toBe(sender);
expect(message.text).toBe(text);
Expand All @@ -129,10 +129,10 @@ describe('Message tests', () => {
it('adds additional text', () => {
const sender = new Sender('NAME');
const text = 'TEXT';
const message = new Message(sender, text);
const message = new Msg(sender, text);
const additionalText = 'MORE';

message.concat(new Message(sender, additionalText));
message.concat(new Msg(sender, additionalText));

expect(message.text).toBe(text + additionalText);
});
Expand Down
19 changes: 7 additions & 12 deletions src/client/src/message.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,19 @@ class Conversation {
if(this.messages.length) {
return this.messages[this.messages.length - 1];
}
return null;
return '';
}
addMessage(message) {
message = Object.assign(new Message(), message);
message = Object.assign(new Msg(), message);
message.sender = Object.assign(new Sender(), message.sender);
let newSender = message.sender;
let lastMessage = this.lastMessage;
// return this;
if (lastMessage && newSender.equals(lastMessage.sender)) {
lastMessage.concat(message);
} else {
this.messages.push(message);
}
// let newSender = message.sender;
Copy link
Owner

@imann24 imann24 Oct 12, 2019

Choose a reason for hiding this comment

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

Commented out code

// let lastMessage = this.lastMessage;
this.messages.push(message);
return this;
}
}

class Message {
class Msg {
constructor(sender, text) {
this.sender = sender;
this.text = text;
Expand All @@ -54,6 +49,6 @@ class Sender {

export {
Conversation,
Message,
Msg,
Sender,
}