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

Erina P. - Kunzite #98

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

Erina P. - Kunzite #98

wants to merge 5 commits into from

Conversation

erinaperez
Copy link

No description provided.

Copy link

@marciaga marciaga left a comment

Choose a reason for hiding this comment

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

Great job! A couple minor comments below but overall nice work!

@@ -1,19 +1,45 @@
import React from 'react';
import './App.css';
import chatMessages from './data/messages.json';
import ChatLog from './components/ChatLog.js';
import { useState } from 'react';

Choose a reason for hiding this comment

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

Since we've already got a line that imports React, we want to avoid adding another line that duplicates it, so better to add to line 1:

import React, { useState } from 'react';

return {
...entry,
liked: !entry.liked,
}; } else {

Choose a reason for hiding this comment

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

this else should be on a new line:

if(id === entry.id) {
  return {
    ...entry,
    liked: !entry.liked,
  };
} else {
...
}      

});
}
const totalLikesTally = entries.reduce((total, entry) => {
if (entry.liked === true) {

Choose a reason for hiding this comment

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

It's ok to compare entry.liked to a boolean but it would be more common to just check for truthinees:

if (entry.liked) {

});
});
}
const totalLikesTally = entries.reduce((total, entry) => {

Choose a reason for hiding this comment

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

Nice use of reduce() here!

{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog
className='chat-log'

Choose a reason for hiding this comment

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

Small thing but these props should be indented two spaces to the right

import ChatEntry from './ChatEntry.js';

const ChatLog = (props) => {
const chatComponents = props.entries.map(entry => {

Choose a reason for hiding this comment

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

Learn showed this pattern of creating an array of React elements and then referencing it in a JSX expression, so it's definitely okay to do here, but I should just add that I have not found it to be common pattern in practice - most people would just return the array in the component's return statement, e.g.

return (
  <div>
    {props.entries.map(entry => (
      <ChatEntry
        key={entry.id} 
        id={entry.id}  
        sender={entry.sender}
        body={entry.body}
        liked={entry.liked}
        timeStamp={entry.timeStamp}
        updateLikedCount={props.updateLikedCount}
      />
    )}
  </div>
);

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