Skip to content

Solution#15

Open
MrHunterBoi wants to merge 2 commits into
mate-academy:masterfrom
MrHunterBoi:develop
Open

Solution#15
MrHunterBoi wants to merge 2 commits into
mate-academy:masterfrom
MrHunterBoi:develop

Conversation

@MrHunterBoi
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@ArtemTeslenko ArtemTeslenko left a comment

Choose a reason for hiding this comment

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

Good job 👍 User should get some information about errors, please handle errors)

Comment thread frontend/src/api.js
try {
return axios.get(`${API_URL}/rooms`);
} catch (err) {
return [];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

when the result of request is not successful you need to inform user about it, return empty array is not good

Comment thread frontend/src/api.js
try {
return axios.get(`${API_URL}/rooms/${id}`);
} catch (err) {
return [];
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the same, check all over the document

import './Chat.css';
import { socket } from '../../webSocket.js';

<h1 className="title">Chat application</h1>;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

do you need this line?

Comment on lines +13 to +14
const [rooms, setRooms] = useState([]);
const [room, setRoom] = useState(null);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it is not a mistake, but it decreases readability, try to avoid using similar names with a difference in one letter (room, rooms, etc.)


export const MessageList = ({ messages }) => (
<ul className="messageList">
{messages.map(message => (
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

destructuring


setRooms(currentRooms => currentRooms.filter(currentRoom => currentRoom.id !== room.id));
} catch (err) {
console.log(err);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

handle error

try {
updateRoom(room.id, room.name);
} catch (err) {
console.log(err);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

handle error

Comment thread src/messages.js
Comment on lines +6 to +16
// {
// id: '1',
// name: `Room #1`,
// messages: [
// {
// time: new Date(),
// text: 'Ye',
// author: 'Max',
// },
// ],
// },
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

delete unnecessary comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants