Conversation
… without creating coutLike state in App.jsx.
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
There was a problem hiding this comment.
👍 Nice use of a shared file to store the proptypes for chat entries. This allows us to avoid duplicating them in ChatEntry and ChatLog.
| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
The id, sender, body, timeStamp, and liked props are always passed (they're defined explicitly in the data and also provided in the test) so we can (and should) mark them isRequired.
The remaining props were up to you, and the tests don't know about them. As a result, using isRequired would cause a warning when running any tests that only pass the known props. As a result, either leaving the prop not marked isRequired or updating the tests to pass a dummy function is required to avoid the warnings.
If we do leave props marked not isRequired, we should either have logic to avoid using those values if they are missing, or make allowance for using a default value.
|
|
||
| ChatEntry.propTypes = { | ||
| // Fill with correct proptypes | ||
| ...messageDataProtoTypes, |
There was a problem hiding this comment.
👍 Great use of spreading to be able to include the base members from the shared entry proptypes, while allowing for additional props to be specified.
| <button className="like">🤍</button> | ||
| <p className={messageColors?.[senderType] ?? ''}>{body}</p> | ||
| <p className="entry-time"><TimeStamp time={timeStamp} /></p> | ||
| <button className="like" onClick={() => onToggleLike(id)}>{liked ? '❤️' : '🤍'} |
There was a problem hiding this comment.
Note that for any props that aren't required, we should check that a value was supplied before trying to use it. For example, trying to call a function reference if the value is undefined will result in an error.
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p className={messageColors?.[senderType] ?? ''}>{body}</p> | ||
| <p className="entry-time"><TimeStamp time={timeStamp} /></p> |
There was a problem hiding this comment.
Nice use of the supplied TimeStamp. We pass in the timeStamp string from the message data and it takes care of the rest. All we had to do was confirm the name and type of the prop it was expecting (which we could do through its PropTypes) and we're all set!
| className={`button-size ${color}`} | ||
| onClick={() => onUpdateColor(sender, color)} | ||
| > | ||
| <p className='button-size'>●</p> |
There was a problem hiding this comment.
Consider making a CONSTANT like const CIRCLE_GLYPH = '●'; and injecting it as { CIRCLE_GLYPH } to make this more readable without the magic number.
|
|
||
| const COLORS = ['red', 'orange', 'yellow', 'green', 'blue', 'purple']; | ||
|
|
||
| const ColorChoice = ({ sender, onUpdateColor }) => { |
There was a problem hiding this comment.
👍 Nice creation of a component to help with picking a color.
Notice that other than the name of the sender parameter, this component is independent of the other notions present in the app. The sender param itself is only used to send back to the change handler, acting as sort of an "id" for this control to identify which instance called the change handler. As such, consider using a more general name, such as id or name (similar to like a button control), or you might see such an "opaque" identifier refered to as a slug.
Passing an identifier like this requires then that the component know that it needs to pass that value along with the selected value to the callback function. That approach works fine, and is similar to what we did for handling changes to the message like. Some other possibilities could involve packaging up the name and new value into an event-like structure, similar to browser-native controls.
Or to avoid needing to have the control need to know that an additional value should be passed, we could instead write this to only call the callback with the color information, but pass different functions to different control instances so that the callback itself knows which sender it was related to. The App could have a function that creates such "bound" functions for us (a function that takes the name info it needs, and returns a function that takes a color value and already has the name info baked into it).
// in App
const makeColorHandlerForName = (name) => {
// returned inner function is given to the color picker component
return (color) => {
// does whatever needs to be done to handle updating the color for name
// both name and color are available to this function
};
};| {COLORS.map((color, index) => { | ||
| return ( | ||
| <button | ||
| key={index} |
There was a problem hiding this comment.
We could even use the color itself as the key. key needn't be an integer. It just needs to be unique.
| Chat Between{' '} | ||
| <span className={messageColors.local}>{localSender}</span>{' '} | ||
| and{' '} | ||
| <span className={messageColors.remote}>{remoteSender}</span> |
There was a problem hiding this comment.
👍 Nice use of the calculated senders to build the header. I see that you also ran into the quirk that JSX really likes to "swallow" whitespace at the start and end of a line (different from plain HTML), requiring the insertion of explicit blanks here and there.
| <section> | ||
| <div> | ||
| <p className={messageColors.local}>{localSender}'s color:</p> | ||
| <ColorChoice sender={localSender} onUpdateColor={updateMessageColor} /> | ||
| </div> | ||
| <h1>{totalLikes} ❤️s</h1> | ||
| <div> | ||
| <p className={messageColors.remote}>{remoteSender}'s color:</p> | ||
| <ColorChoice sender={remoteSender} onUpdateColor={updateMessageColor} /> | ||
| </div> | ||
| </section> |
There was a problem hiding this comment.
Note that adding in the pickers here ends up cutting off the top of the first message. Some additional style tweaks might be necessary to ensure the messages don't start until below the header.
2 participants
No description provided.