work in progress #15

lx93 · 2019-08-19T22:03:25Z

Fixes #6

imported react-chat-ui (https://www.npmjs.com/package/react-chat-ui)
renamed class {Message} in to {Msg} to avoid naming conflict with react-chat-ui

Problem:
right now the chat is always one char behind while chatting with another user. I think it has something to do with react life cycle/state update but couldnt figure out how to resolve.

imann24 · 2019-08-20T05:29:01Z

Looks like some client-side tests are failing

imann24 · 2019-08-20T05:32:10Z

Minor bug, seems like there are empty chat bubbles before any message are sent:

imann24 · 2019-08-20T05:29:36Z

src/client/src/App.js

-                   this.handleSubmit();
-              }
-         }
+         // if (event.key === ' ') {


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

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?

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

Planning to remove this code?

imann24 · 2019-08-20T05:32:56Z

src/client/src/App.js

@@ -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())}/>


Empty console log?

Perhaps some of your changes aren't committed yet?

imann24 · 2019-08-20T05:35:03Z

src/client/src/App.js

-          </div>
+
+            <ChatFeed
+              messages={[new Message({id: 1,message: this.state.conversation.lastMessage.text,senderName:this.state.user.name}),


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

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

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

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?

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?

src/client/src/message.js

imann24 · 2019-08-20T05:38:18Z

package.json

@@ -18,5 +18,8 @@
     "devDependencies": {
          "concurrently": "^4.0.1",
          "recursive-install": "^1.4.0"
+     },
+     "dependencies": {
+          "react-chat-ui": "^0.3.2"


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)

imann24 · 2019-08-20T05:42:33Z

I think the issue bug you mentioned may have something to do with needing to invoke this.setState whenever a change occurs in the data model

lx93 · 2019-08-20T14:19:08Z

okay i will try to amend the concerns raised in your comments!

imann24 · 2019-08-20T14:35:29Z

okay i will try to amend the concerns raised in your comments!

Sounds good! I was just teasing you about the Message to Msg one. Your class name seems more JS-like anyway!

I really like the React messaging UI. Looks very clean

imann24

A few more things on a second review! Always worth looking again when you've actually had some coffee 😅 . Anyways, great PR!

imann24 · 2019-08-20T15:57:56Z

src/client/src/App.js

@@ -47,6 +51,8 @@ class App extends Component {
            <header className="App-header">
              <img src={logo} className="App-logo" alt="logo" />
            </header>
+
+


Nice spacing for visual clarity!

imann24 · 2019-08-20T15:58:22Z

src/client/src/App.js

-            })}
-          </div>
+
+            <ChatFeed


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

imann24 · 2019-08-20T15:58:51Z

src/client/src/App.js

-          </div>
+
+            <ChatFeed
+              messages={[new Message({id: 1,message: this.state.conversation.lastMessage.text,senderName:this.state.user.name}),


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

imann24 · 2019-08-20T16:00:20Z

src/client/src/App.js

+         //           this.handleSubmit();
+         //      }
+         // }
+         this.handleSubmit();


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)

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.

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

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

imann24

Good code cleanup! May still have a few notes to address though

imann24 · 2019-10-12T18:24:26Z

package.json

@@ -18,5 +18,8 @@
     "devDependencies": {
          "concurrently": "^4.0.1",
          "recursive-install": "^1.4.0"
+     },
+     "dependencies": {
+          "react-chat-ui": "^0.3.2"


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)

imann24 · 2019-10-12T18:25:31Z

src/client/src/App.js

-                   this.handleSubmit();
-              }
-         }
+         // if (event.key === ' ') {


Planning to remove this code?

imann24 · 2019-10-12T18:27:25Z

src/client/src/App.js

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


Believe we still need a call to update the React state

which state should we update in handleSubmit?

this.state.conversation and this.state.text

imann24 · 2019-10-12T18:28:28Z

src/client/src/App.js

@@ -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())}/>


Perhaps some of your changes aren't committed yet?

imann24 · 2019-10-12T18:29:44Z

src/client/src/message.js

-        } else {
-            this.messages.push(message);
-        }
+        // let newSender = message.sender;


Commented out code

imann24 · 2019-10-12T18:40:09Z

package.json

@@ -18,5 +18,8 @@
     "devDependencies": {


Having some functional issues when I test the app:

Not able to send messages with this version

When I start the chat there are empty chat bubbles

Think it would preferable to aim for the flow that we create a new chat bubble when the other person starts speaking

work in progress

c8599f1

imann24 requested changes Aug 20, 2019

View reviewed changes

imann24 reviewed Aug 20, 2019

View reviewed changes

imann24 self-requested a review August 20, 2019 16:00

lx93 added 2 commits October 12, 2019 01:48

refactored the test file, but still failing one test. please review.

9573e94

refactored the test file, but still failing one test. please review.

9a569d8

imann24 reviewed Oct 12, 2019

View reviewed changes

work in progress #15

Are you sure you want to change the base?

work in progress #15

Conversation

lx93 commented Aug 19, 2019 • edited by imann24 Loading

imann24 commented Aug 20, 2019

imann24 commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imann24 Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imann24 commented Aug 20, 2019

lx93 commented Aug 20, 2019

imann24 commented Aug 20, 2019 • edited Loading

imann24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lx93 Aug 20, 2019 • edited by imann24 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imann24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lx93 commented Aug 19, 2019 •

edited by imann24

Loading

imann24 Aug 20, 2019 •

edited

Loading

imann24 commented Aug 20, 2019 •

edited

Loading

lx93 Aug 20, 2019 •

edited by imann24

Loading