Skip to content

C22 - Phoenix - Kristina Nguyen #26

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 4 commits into
base: main
Choose a base branch
from

Conversation

kstinanguyen
Copy link

No description provided.

Copy link
Collaborator

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Great work! Let me know if you have questions on any of the feedback ^_^

<h1>Chatroom: {chatData[0].sender} and {chatData[1].sender}</h1>
<section id='heartWidget'>
<span className='widget'>{likedCount} ❤️s</span>
<DarkMode currentTheme={theme} toggleTheme={toggleDarkMode}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love the theme toggle!

setTheme(mode);
};

const likedCount = chatData.filter((chat) => chat.liked).length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great work calculating the hearts count from the chatsData! Since we don't need the contents of the array we get from filter, another option is to use a higher order function like array.reduce to take our list of messages and reduce it down to a single value.

// This could be returned from a helper function
// totalLikes is a variable that accumulates a value as we loop over each entry in chatEntries
const likedCount = chatEntries.reduce((totalLikes, currentMessage) => {
    // If currentMessage.liked is true add 1 to totalLikes, else add 0
    return (totalLikes += currentMessage.liked ? 1 : 0);
}, 0); // The 0 here sets the initial value of totalLikes to 0

Comment on lines +14 to +17
if (chat.id === id) {
return { ...chat, liked: !chat.liked };
} else {
return chat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice managing of data & making sure we return a new object when we need to alter a message in our list.

Comment on lines +22 to +24
const toggleDarkMode = (mode) => {
setTheme(mode);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we aren't taking any other actions with the mode argument, I think we could factor out the toggleDarkMode function and pass setTheme directly to the DarkMode component.

Comment on lines +9 to +26
const [chatData, setChatData] = useState(CHATS);
const [theme, setTheme] = useState('App');

const handleLikedMessages = (id) => {
setChatData(chatData => chatData.map(chat => {
if (chat.id === id) {
return { ...chat, liked: !chat.liked };
} else {
return chat;
}
}));
};

const toggleDarkMode = (mode) => {
setTheme(mode);
};

const likedCount = chatData.filter((chat) => chat.liked).length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It can be a little easier to read and understand code when we co-locate variables rather than placing them in-between functions. One possible re-organization could look like:

  const [chatData, setChatData] = useState(CHATS);
  const [theme, setTheme] = useState('App');
  const likedCount = chatData.filter((chat) => chat.liked).length;

  const handleLikedMessages = (id) => {
    setChatData(chatData => chatData.map(chat => {
      if (chat.id === id) {
        return { ...chat, liked: !chat.liked };
      } else {
        return chat;
      }
    }));
  };

  const toggleDarkMode = (mode) => {
    setTheme(mode);
  };


const filledHeart = liked ? '❤️' : '🤍';

const setMessageBubble = id%2 ? 'local' : 'remote';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const setMessageBubble = id%2 ? 'local' : 'remote';
const setMessageBubble = id % 2 ? 'local' : 'remote';

</section>
</div>
<>
<div className={`chat-entry ${setMessageBubble}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great use of a variable and interpolated string to form the className value.

Comment on lines +31 to +36
id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
liked: PropTypes.bool.isRequired,
onLiked: PropTypes.func.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice use of Prop-Types and flagging those necessary for render with isRequired!.

import ChatEntry from './ChatEntry';

const ChatLog = ({ entries, onLiked }) => {
const chatEntryComponents = entries.map((entry) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice mapping! 🙌🏻

};

ChatLog.propTypes = {
entries: PropTypes.arrayOf(PropTypes.shape({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice use of PropTypes.arrayOf and PropTypes.shape!

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