-
Notifications
You must be signed in to change notification settings - Fork 35
C24 Possum: Makiko Yokoyama #13
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
base: main
Are you sure you want to change the base?
Changes from all commits
974a112
fd5e26a
9a933bd
ba0ddcb
54ed26f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,76 @@ | ||
| import { useState } from 'react'; | ||
| import messages from './data/messages.json'; | ||
| import ChatLog from './components/ChatLog'; | ||
| import ColorChoice from './components/ColorChoice'; | ||
| import './App.css'; | ||
|
|
||
| const numOfLikes = (messageData) => { | ||
| return messageData.reduce( | ||
| (count, message) => { | ||
| return message.liked ? count + 1 : count; | ||
| }, | ||
| 0 | ||
| ); | ||
| }; | ||
|
|
||
| const App = () => { | ||
| const [messageData, setMessageData] = useState(messages); | ||
| const [messageColors, setMessageColors] = useState({ local: 'green', remote: 'blue' }); | ||
|
|
||
| const localSender = messageData[0]?.sender; | ||
| const remoteSender = messageData.find(message => message.sender !== localSender)?.sender; | ||
|
Comment on lines
+20
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Picking out a local an remote sender works well in this example. Also think about how this could be generalized to a conversation with more than two participants (group chat)! |
||
|
|
||
| const handleLikeStatus = messageId => { | ||
| setMessageData((prevMessages) => ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice use of the callback setter style. In this application, it doesn't really matter whether we use the callback style or the value style, but it's good practice to get in the habit of using the callback style. |
||
| prevMessages.map(message => | ||
| message.id === messageId | ||
| ? { ...message, liked: !message.liked } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We showed this approach in class, but technically, we're mixing a few responsibilities here. Rather than this function needing to know how to change the liked status itself, we could move this update logic to a helper function. This would better mirror how we eventually update records when there's an API call involved. In this project, our messages are very simple objects, but if we had more involved operations, it could be worthwhile to create an actual class with methods to work with them, or at least have a set of dedicated helper functions to centralize any such mutation logic. |
||
| : message | ||
| ) | ||
| )); | ||
| }; | ||
|
|
||
| const getSenderType = (senderName) => ( | ||
| senderName === localSender ? 'local' : 'remote' | ||
| ); | ||
|
|
||
| const updateMessageColor = (sender, color) => { | ||
| const senderType = getSenderType(sender); | ||
| setMessageColors(prevColors => ( | ||
| { ...prevColors, [senderType]: color } | ||
| )); | ||
| }; | ||
|
|
||
| const totalLikes = numOfLikes(messageData); | ||
|
|
||
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Application title</h1> | ||
| <h1> | ||
| Chat Between{' '} | ||
| <span className={messageColors.local}>{localSender}</span>{' '} | ||
| and{' '} | ||
| <span className={messageColors.remote}>{remoteSender}</span> | ||
|
Comment on lines
+50
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 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. |
||
| </h1> | ||
| <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> | ||
|
Comment on lines
+55
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| </header> | ||
| <main> | ||
| {/* Wave 01: Render one ChatEntry component | ||
| Wave 02: Render ChatLog component */} | ||
| <ChatLog | ||
| entries={messageData} | ||
| onToggleMessageLike={handleLikeStatus} | ||
| messageColors={messageColors} | ||
| getSenderType={getSenderType} | ||
| /> | ||
| </main> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,4 +97,5 @@ button { | |
|
|
||
| .chat-entry.remote .entry-bubble:hover::before { | ||
| background-color: #a9f6f6; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,41 @@ | ||
| import PropTypes from 'prop-types'; | ||
| import './ChatEntry.css'; | ||
| import TimeStamp from './TimeStamp'; | ||
| import { messageDataProtoTypes, messageColorsProtoTypes } from './sharedPropTypes'; | ||
|
|
||
| const ChatEntry = ({ | ||
| id, | ||
| sender, | ||
| body, | ||
| timeStamp, | ||
| liked, | ||
| onToggleLike, | ||
| messageColors, | ||
| getSenderType, | ||
| }) => { | ||
|
|
||
| const senderType = getSenderType && getSenderType(sender); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing down a "classifier" function like this does change where the logic is defined (and the classifier can be independently tested), but this still requires that So rather than passing down a classifier like this along with the raw message data, we can make a new type that in addition to the raw message data also includes per-message information needed for rendering, such as whether the message should be considered local or remote. This new data type serves as a "model" for the "view", which we can call a "ViewModel". The non-React business logic would be responsible for this conversion, leaving React only responsible for interpreting the results (e.g., whether the message viewmodel indicates the entry should be local). This helps us keep as much business logic out of the React rendering logic as possible. |
||
|
|
||
| const ChatEntry = () => { | ||
| return ( | ||
| // Replace the outer tag name with a semantic element that fits our use case | ||
| <replace-with-relevant-semantic-element className="chat-entry local"> | ||
| <h2 className="entry-name">Replace with name of sender</h2> | ||
| <section className={`chat-entry ${senderType}`}> | ||
| <h2 className="entry-name">{sender}</h2> | ||
| <section className="entry-bubble"> | ||
| <p>Replace with body of ChatEntry</p> | ||
| <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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of the supplied |
||
| <button className="like" onClick={() => onToggleLike(id)}>{liked ? '❤️' : '🤍'} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| </button> | ||
| </section> | ||
| </replace-with-relevant-semantic-element> | ||
| </section> | ||
| ); | ||
| }; | ||
|
|
||
| ChatEntry.propTypes = { | ||
| // Fill with correct proptypes | ||
| ...messageDataProtoTypes, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 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. |
||
| onToggleLike: PropTypes.func, | ||
| messageColors: PropTypes.shape( | ||
| messageColorsProtoTypes | ||
| ), | ||
| getSenderType: PropTypes.func, | ||
| }; | ||
|
|
||
| export default ChatEntry; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,3 +2,5 @@ | |
| margin: auto; | ||
| max-width: 50rem; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import PropTypes from 'prop-types'; | ||
| import './ChatLog.css'; | ||
| import ChatEntry from './ChatEntry'; | ||
| import { messageDataProtoTypes, messageColorsProtoTypes } from './sharedPropTypes'; | ||
|
|
||
| const ChatLog = ({ | ||
| entries, | ||
| onToggleMessageLike, | ||
| messageColors, | ||
| getSenderType, | ||
| }) => { | ||
|
|
||
| return ( | ||
| <div className='chat-log'> | ||
| {entries.map(message => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of |
||
| <ChatEntry | ||
| key={message.id} | ||
| id={message.id} | ||
|
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 The |
||
| sender={message.sender} | ||
| body={message.body} | ||
| timeStamp={message.timeStamp} | ||
| liked = {message.liked} | ||
| onToggleLike={onToggleMessageLike} | ||
| messageColors={messageColors} | ||
| getSenderType={getSenderType} | ||
| />)} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| ChatLog.propTypes = { | ||
| entries: PropTypes.arrayOf(PropTypes.shape({ | ||
| ...messageDataProtoTypes | ||
| })).isRequired, | ||
| onToggleMessageLike: PropTypes.func, | ||
| messageColors: PropTypes.shape( | ||
| messageColorsProtoTypes | ||
| ), | ||
| getSenderType: PropTypes.func, | ||
| }; | ||
|
|
||
| export default ChatLog; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| .color-container { | ||
| display: flex; | ||
| flex-direction: row; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| .button-size { | ||
| font-size: 2rem; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import PropTypes from 'prop-types'; | ||
| import './ColorChoice.css'; | ||
|
|
||
| const COLORS = ['red', 'orange', 'yellow', 'green', 'blue', 'purple']; | ||
|
|
||
| const ColorChoice = ({ sender, onUpdateColor }) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice creation of a component to help with picking a color. Notice that other than the name of the 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
};
}; |
||
| return ( | ||
| <div className="color-container"> | ||
| {COLORS.map((color, index) => { | ||
| return ( | ||
| <button | ||
| key={index} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could even use the |
||
| className={`button-size ${color}`} | ||
| onClick={() => onUpdateColor(sender, color)} | ||
| > | ||
| <p className='button-size'>●</p> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making a CONSTANT like |
||
| </button> | ||
| ); | ||
| })} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| ColorChoice.propTypes = { | ||
| sender: PropTypes.string.isRequired, | ||
| onUpdateColor: PropTypes.func.isRequired | ||
| }; | ||
|
|
||
| export default ColorChoice; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice use of a shared file to store the proptypes for chat entries. This allows us to avoid duplicating them in ChatEntry and ChatLog. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| export const messageDataProtoTypes = { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd avoid using the naming export const messageDataPropTypes = {or export const messageDataShape = { |
||
| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
|
Comment on lines
+4
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The The remaining props were up to you, and the tests don't know about them. As a result, using If we do leave props marked not |
||
| }; | ||
|
|
||
| export const messageColorsProtoTypes = { | ||
| local: PropTypes.string.isRequired, | ||
| remote: PropTypes.string.isRequired, | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice job determining the total likes based on the like data of each message. We don't need an additional piece of state to track this, since it can be derived from the existing state we are tracking.
I like that you wrapped your
reducecall in a well-named helper function. This is one way that we can make usingreducea little more understandable for other programmers reading our code, since the syntax can be a little confusing.Since the message data is passed in, we could even relocate this function completely out of this function. This is definitely business logic, not rendering logic. So it can be managed separately from our React components.