Skip to content
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

Week 11: Project Happy thoughts #98

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jacquelinekellyhunt
Copy link

@jacquelinekellyhunt jacquelinekellyhunt commented Oct 27, 2024

Copy link
Contributor

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

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

Hi Kelly!
Before I forget: Next week I'd like to see you commit more often! Two commits for a project this big seems a bit scary - try to commit whenever you've introduced a new feature.

Onto the code ⚡
Some of your code is quite complex and I think you could benefit from more comments or potentially breaking the code up a bit more with descriptive variables. Oneliners are short but not always the easiest to understand 😅 Example:

const handleLike = (thoughtId) => {
    if (!likedThoughts.includes(thoughtId)) {
      setLikedThoughts((prevLikes) => [...prevLikes, thoughtId]);
      setThoughts((prevThoughts) =>
        prevThoughts.map((thought) =>
          thought._id === thoughtId
            ? { ...thought, hearts: thought.hearts + 1 }
            : thought
        )
      );
    }
  };

With that said - kudos for disabling the like function for already liked posts 🥳 and overall, your code is easy to follow and you have a nice structure. Also kudos for doing your own take on the design but still keeping the big features!

Some notes about clean code

  • No need to import React
  • Out of curiousity, why did you change the App import type to not match the rest of your components? 👀

Continue like this!

@jacquelinekellyhunt
Copy link
Author

@HIPPIEKICK I committed my Typescript changes straight to the main accidentally - I was not in the best shape last week as a lot happened so I might not have done everything right in terms of turning the work in - please let me know if I need to do anything else to get this approved.

https://happiestthoughts.netlify.app/

Copy link

@JennieDalgren JennieDalgren left a comment

Choose a reason for hiding this comment

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

Great job!

You have converted your code to TypeScript and implemented interfaces in a great way!
All approved

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.

3 participants