Repository navigation
Manchester | 26-Jun-SDC | Fithi Teklom | Sprint 2 | Chat App - #125
Fithi-Teklom wants to merge 7 commits into
Conversation
Fithi-Teklom
added
the
Needs Review
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work on this task, your deployed app works pretty well.
I've got some comments for the submitted code for you to look at.
| </head> | ||
|
|
||
| <body> | ||
| <h1>Chat App</h1> |
There was a problem hiding this comment.
Did you run the lighthouse tool to check your HTML? Does it offer any suggestions for accessibility fixes?
| @@ -0,0 +1 @@ | |||
| node_modules | |||
There was a problem hiding this comment.
This .gitignore has the same content as the top level .gitignore, do you need both of them?
| messageElement.textContent = `${message.username}: ${message.text}`; | ||
| messageElement.style.color = message.color; | ||
|
|
||
| // const likeButton = document.createElement("button"); |
There was a problem hiding this comment.
We want to avoid commented out code like this in PRs. Is there a reason you left the old code here?
| id: messages.length, | ||
| username: username.trim(), | ||
| text: text.trim(), | ||
| color: color, |
There was a problem hiding this comment.
One thing to be careful of here is you are accepting potentially arbitrary content. In your HTML, there are only a few specific colours you offer, but it's possible to "hack" the page to send any colour. If you intended there to be only a set selection of colours, you probably want to validate it at the backend. What do you think?
| messages.push(message); | ||
|
|
||
| while (callbacksForNewMessages.length > 0) { | ||
| const callback = callbacksForNewMessages.pop(); |
There was a problem hiding this comment.
What do you think are the advantages / disadvantages of the approach you've taken here for updating clients?
LonMcGregor
added
Reviewed
2 participants
Self checklist
Task code
CYF-1162
Chat App
Features implemented
Deployed application
Frontend:
https://fithi-chat-app-frontend.trainees.hosting.cyf.academy/
Backend:
https://fithi-chat-app-backend.trainees.hosting.cyf.academy/