-
Notifications
You must be signed in to change notification settings - Fork 8
YANA_SENIUK-w2-React #18
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?
Conversation
I have already know my mistakes. I will fix it soon. |
@yanaesher How are you progressing with the next commit? I was going to start reviewing your code but if you want me to wait for your fixes then I can. Also asked in Slack. |
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.
Hi Yana,
Well done for quickly cleaning up the issues we discussed via Slack yesterday. Your code is organised very well! This will look impressive when showing to potential employers.
I have left a couple of comments to make your code look even more organised.
|
||
const BASE_STORE_URL = "https://fakestoreapi.com"; | ||
|
||
export const getAllCategories = async () => { |
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.
Really nicely organised fetcher fns!
const product = await getProductById(id!); | ||
setProduct(product); | ||
} catch (err) { | ||
if (err instanceof Error) setError(err.message); |
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 can do this, but it's not really necessary.
You're also excluding other error types. What happens here if there's some error that is not an instance of Error
? 🔵
@@ -0,0 +1,5 @@ | |||
export const getFetcher = async (url: string) => { | |||
const res = await fetch(url); | |||
if (!res.ok) throw new Error(`Failed to fetch data: ${res.status}`); |
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 make this a little more user-friendly - users won't understand what the status codes mean. 🔵
|
||
return ( | ||
<div className="wrapper"> | ||
<main className="main"> |
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.
Are these class names in use?
|
||
{error && <p className="text-red-500">{error}</p>} | ||
|
||
{!loading && !error && categories.length > 0 && ( |
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 don't need to do the truthiness check for loading
and error
here. You've already verified on the previous lines that loading
and error
must both be falsy in order to reach this line of code. If you add in every single variable in a large application your JSX will soon become difficult to read and manage. 🟠
@@ -0,0 +1,25 @@ | |||
import type { Category } from "../types/category"; |
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.
This no longer exists 🟠
No description provided.