-
Notifications
You must be signed in to change notification settings - Fork 434
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
Happy thoughts Josefin R #438
base: master
Are you sure you want to change the base?
Conversation
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.
Great work on the project! Overall a well done project, it shows that you understand useEffect and React,
Added a few comments below, but no big things. Be proud!
|
||
useEffect(() => { | ||
updateThoughtsList(); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
Is there a reason why you didn't use eslint-disable in the whole file? Makes it easier (in my own opinion) to remove if necessary in the top level of the file.
return ( | ||
<div className="inputForm"> | ||
<form onSubmit={handleFormSubmit}> | ||
<h3>What's making you happy right now?</h3> |
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.
&apos
Nice use off Predefined Character Entities (had to look up that name😅 )
code/src/components/SingleThought.js
Outdated
const SingleThought = ({ thought, onHeartClick }) => { | ||
const date = new Date(thought.createdAt); | ||
const timeDiff = Math.round((new Date() - date) / (1000 * 60)); | ||
if (timeDiff < 1) { | ||
timeStamp = 'just now'; | ||
} else if (timeDiff < 90) { | ||
timeStamp = `${timeDiff} min ago`; | ||
} else { | ||
const hoursDiff = Math.round(timeDiff / 60); | ||
timeStamp = `${hoursDiff} hour${hoursDiff > 1 ? 's' : ''} ago`; | ||
} |
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.
A very nice structured function; Its easy to understand.
code/src/components/InputForm.js
Outdated
onChange={handleInputChange} | ||
maxLength="140" /> | ||
<div id="counter">140/{charCount}</div> | ||
<button type="submit"><span id="heart"> Send Happy Thought </span></button> |
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.
You might want to add a feature that disables the send button if a value (<= 5) is entered in the text box;, to prevent a error message from the API, that gets rendered out in the DataList component.
No description provided.