Skip to content

C22_Sphinx_Rong Jiang #28

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

C22_Sphinx_Rong Jiang #28

wants to merge 5 commits into from

Conversation

cornetto9
Copy link

No description provided.

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

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

Nice job on react-chatlog!

Comment on lines +4 to +8
import messages from './data/messages.json'


// import data from messages.json
const LOG = messages;

Choose a reason for hiding this comment

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

You can directly name messages as LOG on line 4.

Suggested change
import messages from './data/messages.json'
// import data from messages.json
const LOG = messages;
import LOG from './data/messages.json';

Be sure to terminate a statement with semi-colons.

setEntries(updatedEntries);
}

const totalLikes = entries.filter(entry => entry.liked).length;

Choose a reason for hiding this comment

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

Typically, you'd see reduce used to add up some values.

const totalLikes = entries.reduce((totalLikes, entry) => (totalLikes + entry.liked);

<p className="entry-time">Replace with TimeStamp component</p>
<button className="like">🤍</button>
<p className='body'>{body}</p>
<TimeStamp time = {timeStamp}></TimeStamp>

Choose a reason for hiding this comment

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

Nice job using the provided Timestamp component here.

Convention is to not have spaces around = sign when passing props

Suggested change
<TimeStamp time = {timeStamp}></TimeStamp>
<TimeStamp time={timeStamp}></TimeStamp>

import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = ({id, sender, body, timeStamp,liked, onLikeToggle}) => {

Choose a reason for hiding this comment

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

Suggested change
const ChatEntry = ({id, sender, body, timeStamp,liked, onLikeToggle}) => {
const ChatEntry = ({ id, sender, body, timeStamp,liked, onLikeToggle }) => {

Spaces around curly braces when destructuring

<p className='body'>{body}</p>
<TimeStamp time = {timeStamp}></TimeStamp>
<button
className='like' //className from test

Choose a reason for hiding this comment

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

Prefer not to have comments in code unless absolutely necessary.


if (sender === 'Vladimir') {
isLocalUser = true;
}

Choose a reason for hiding this comment

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

Suggested change
}
} ;

Linter calls out that you're missing semi-colons. if you haven't configured your linter, you might want to do that so it can highlight syntax/style issues.

Comment on lines +11 to +15
let isLocalUser = false

if (sender === 'Vladimir') {
isLocalUser = true;
}

Choose a reason for hiding this comment

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

Would prefer to have this logic put in a method and the method get called somewhere.

Suggested change
let isLocalUser = false
if (sender === 'Vladimir') {
isLocalUser = true;
}
const setLocalOrRemote () => {
return sender == 'Vladimir' ? 'local' : 'remote';
};

Then on line 18, you can call the function:

    <div className={`chat-entry ${setLocalOrRemote()}`}>

import ChatEntry from './ChatEntry';


const ChatLog = ({entries, onLikeToggle}) => {

Choose a reason for hiding this comment

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

Suggested change
const ChatLog = ({entries, onLikeToggle}) => {
const ChatLog = ({ entries, onLikeToggle }) => {

Comment on lines +7 to +22
return (
<div className="chat-log">
{entries.map(entry => (
<ChatEntry
key={entry.id}
id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
onLikeToggle={onLikeToggle}
/>
))}
</div>
);
};
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

It's more typical to see the result of using map referenced by a variable and using that variable in the JSX, like:

    const chatLogs =  entries.map((entry) => {
	      return (
	          <ChatEntry                                 
	            key={entry.id}
	            id={entry.id}
	            sender={entry.sender}
	            body={entry.body}
	            timeStamp={entry.timeStamp}
	            liked={entry.liked}
	            handleLike={handleLike}
	            currentUser={currentUser}
	          />
	        );
	     });
    };

  return (
        <div className="chat-log">
            {chatLogs}
        </div>
  );

import TimeStamp from './TimeStamp';

const ChatEntry = ({id, sender, body, timeStamp,liked, onLikeToggle}) => {

Choose a reason for hiding this comment

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

Linter says "Block must not be padded by blank lines". Remove line 6

Suggested change

Choose a reason for hiding this comment

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

Uploading image.png…
My linter shows all the style issues here. See the pink and green squiggley lines. I recommend configuring your linter so you can catch these issues before opening a PR.

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