Skip to content
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

Sharks/Bahareh #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sharks/Bahareh #42

wants to merge 1 commit into from

Conversation

b-izad
Copy link

@b-izad b-izad commented Jun 23, 2022

No description provided.

@@ -1,16 +1,63 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do this instead:

Suggested change
import React from 'react';
import React, {useState} from 'react';

@@ -1,16 +1,63 @@
import React from 'react';
import './App.css';
import chatMessages from './data/messages.json';
import { useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { useState } from 'react';

Comment on lines +12 to +18
const updateMessageData = (updatedMessage) => {
const messages = messagesInfo.map((message) =>
message.id === updatedMessage.id ? updatedMessage : message
)

setMessagesInfo(messages);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I might consider refactoring this, so that it doesn't update ALL of the message data, just the liked property

Comment on lines +20 to +33
// const updateMessageData = (updatedMessage) => {
// const messages = messagesInfo.map((message) => {
// if (message.id === updatedMessage.id) {
// return updatedMessage;
// } else {
// return message;
// }
// });

// setMessagesInfo(messages);



// };
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// const updateMessageData = (updatedMessage) => {
// const messages = messagesInfo.map((message) => {
// if (message.id === updatedMessage.id) {
// return updatedMessage;
// } else {
// return message;
// }
// });
// setMessagesInfo(messages);
// };

Comment on lines +53 to +54


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +9 to +14
const onLikeButtonClick = () => {

const updatedMessage = { ...props, liked: !props.liked }
props.updateMessageData(updatedMessage);

};
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm you are giving this function a lot of editing power when it only needs to update the liked property. Think about giving the function access only to liked.

<p className="entry-time">Replace with TimeStamp component</p>
<button className="like">🤍</button>
<p>{props.body}</p>
<p className="entry-time">{moment(props.timeStamp).fromNow()}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be using the TimeStamp component that was given to us, not import a new library moment.

This works! Great job figuring out a way to implement this on your own, though!

@@ -0,0 +1,40 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import React from 'react';
import React, {useState} from 'react';

import './ChatEntry.css';
import ChatEntry from './ChatEntry';
import PropTypes from 'prop-types';
import { useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { useState } from 'react';

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