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

Atk technical #13

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

Conversation

ToddLGarrison
Copy link

  • Create TrendingList component to render list of trending items from getTrending api.
  • Implement TrendingList component on App.tsx
  • Add try/catch block to getTrending fetch, allow for handing of successful and unsuccessuf/error/empty response
  • Add try/catch to useEffect in App.tsx to provide clear error messaging fetchTrending function
  • Resolved form input (handleNameUpdate) not updating.
  • Implement useMemo() on ContentContainer.tsx to resolve performance issue due to numOfFactors() function. Required removal of () on line19. Function is called within numOfFactors on line 14.

setTrendingRecipes(trending)
setErrorMessage(null)
} catch (error) {
console.error('Error in fetching trending data: ', error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A potential error message on this console could be "Error in fetching trending data: Error in fetching trending data: ...status" since you have the error message format set up already in the api function try catch.

Copy link
Collaborator

@timvvoodman timvvoodman left a comment

Choose a reason for hiding this comment

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

just one small thing that is very nit-picky, nice job!

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