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

Arnau's happy thoughts project #71

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

Conversation

vidalhuix
Copy link

Netlify link

here

Collaborators

[vidalhuix]

Copy link
Contributor

@dzc1 dzc1 left a comment

Choose a reason for hiding this comment

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

Bona tarda Arnau,

Great work overall, I will give you the passing grade since all requirements have been met and the logic and prop destructuring within your App.jsx file is great with 1 exception. If you noticed when you post a new thought within your app, you have to manually refresh the page, to avoid doing within your PostWall.jsx this just add a variable to your dependency array from this:

  useEffect (() => {
    fetchThoughts()
  }, []) //Only run once when the component mounts

to this:

  useEffect (() => {
    fetchThoughts()
  }, [thoughts]) //Only run once when the component mounts

This will do the job!!

Keep up the great work!

@vidalhuix
Copy link
Author

vidalhuix commented Mar 28, 2024

Bon migdia Diego, quin bon rotllo rebre els reviews així:)

I have done the changes and I have added the character count and the loading state too. For some reason the technigo logo looks ok on "npm run dev" but not in the browser. Any idea on how to solve that issue?

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