Skip to content

Conversation

Lindseymarcea
Copy link

No description provided.

Copy link

@chimerror chimerror left a comment

Choose a reason for hiding this comment

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

Good work!

I didn't have very many comments, but I hope that given that you've seen how picky I can be that as few comments from me as possible is a good thing. There's a slight difference that causes a test failure, and a missing prop from your PropTypes for ChatLog, and some guidance on the StrictMode code you commented out, but those are all minor issues, and my other comment is positive!

The code is cleanly written and correctly implemented (save that missing space), so that's good enough for a Green!

Comment on lines +9 to +15
// root.render(
// <React.StrictMode>
// <App />
// </React.StrictMode>
// );
// Not sure if I need this code but it was in the live code, code gets sent into render method and once its rendered its injected
// directly into html inside the anchor div

Choose a reason for hiding this comment

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

You don't need this code, and I verified that it works without it, but there is an advantage to keeping it. Namely, it will render the App component in strict mode, which doesn't produce any UI, but will produce warnings that may help you find bugs.

You can read more about it in the React docs on Strict Mode.

<header>
<h1>Application title</h1>
<h1>Chat with Estragon and Vladimir</h1>
<div> {updateLike()}❤️s</div>

Choose a reason for hiding this comment

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

As mentioned in my comment on Learn, this causes a test failure because our tests are expecting "3 ❤️s" with a space in between the number and the heart, while your code produces "3❤️s".

But this is a relatively minor issue, obviously.

Comment on lines +13 to +15
className={`chat-entry ${
props.sender === 'Estragon' ? 'remote' : 'local'
} `}

Choose a reason for hiding this comment

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

This works to distinguish the remote bubbles! Good job!

body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
toggleHeartCallback={props.toggleHeartCallback}

Choose a reason for hiding this comment

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

Since you use this toggleHeartCallback prop, it's likely a good idea to include it in your PropTypes below.

I would even recommend that it be .isRequired because this callback is pretty vital to the function of the code. However, that would cause issues with our tests, that do not supply this callback, so it'd be OK without .isRequired.

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