Project reat homework w2 #26
Conversation
There was a problem hiding this comment.
Hello Christina, thank you for your work this week!
There are still a few things I'd like you to improve before I can approve your work.
The main one is the missing product detail page. I'm not sure if you forgot to commit some of your changes or if you didn't know how to approach this task? Either way is fine. If you simply forgot to upload your changes, please do so and let me know when I can review them. If you didn't know how to go about creating the product detail page, feel free to reach out on Slack and I'll can give you more detailed instructions.
At a high level:
- You will need to update the
App.jsfile to complete the definition of the dedicated route - I have left a comment with an example there. - You will need to create a React component that render to content of the product detail page according to the homework instructions. In this component, you can use the
useParamshook to know what is the ID of the product that should be loaded from the API endpoint. - Each of the product tiles on the homepage should lead to a corresponding product detail page. You will need to find a way of making the product ID available as a prop in the
Productcomponent. You can then use for example theuseNavigatehook from the React Router Dom package to handle the navigation.
Secondly, I have added a few comments around the handling of the loading state of the CategoryList and ProductList components. The main idea is that the page should visually indicate when the app is waiting for the response from the API/server. This also applies when switching between the product categories.
Lastly, you can simplify your component structure when you move the contents of the ProductsBlock into your Home component and delete the ProductBlock component.
| import ProductsBlock from "../components/Products/ProductsBlock"; | ||
|
|
||
| const Home = () => { | ||
| return <ProductsBlock />; |
There was a problem hiding this comment.
You don't need to use the ProductsBlock component here. Instead, load the ProductList and CategoryList components directly here and move the content of the ProductBlock component into this file instead. This will help you keep the application structure more streamlined.
| const [loading, setLoading] = useState(false); | ||
| const [error, setError] = useState(null); | ||
|
|
||
| function fetchCategories() { | ||
| fetch("https://fakestoreapi.com/products/categories") | ||
| .then((res) => { | ||
| setLoading(true); | ||
| return res.json(); | ||
| }) | ||
| .then((data) => { | ||
| setLoading(false); | ||
| setCategories(data); | ||
| }) | ||
| .catch((error) => { | ||
| setLoading(false); | ||
| setError("Can not fetch data!"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This way, the loading state is never true. You can either initialize the loading variable with true value when declaring the useState hook: const [loading, setLoading] = useState(false); or call setLoading(true) at the beginning of the fetchCategories function. You can then remove line 11 as it's not necessary anymore.
|
|
||
| const ProductList = ({ category }) => { | ||
| const [products, setProducts] = useState([]); | ||
| const [loading, setLoading] = useState(false); |
There was a problem hiding this comment.
Similar feedback to the CategoryList component - I recommend initializing the loading variable with a true value.
| function fetchProducts(url) { | ||
| fetch(url) | ||
| .then((res) => { | ||
| setLoading(true); |
There was a problem hiding this comment.
If loading is initialized as true this line is not necessary.
| } | ||
|
|
||
| useEffect(() => { | ||
| if (category) { |
There was a problem hiding this comment.
To show the loading status also when users switch between the categories, set the loading variable to true at the beginning of this useEffect hook.
week2/project/ecommerce/src/App.js
Outdated
|
|
||
| const router = createBrowserRouter([ | ||
| { path: "/", element: <Home /> }, | ||
| { path: "product:id" }, |
There was a problem hiding this comment.
You might have forgotten to push some changes? I would expect here that the product/:id path will point to an element that will display the product detail page.
{ path: "/product/:id", element: <ProductDetails /> }
There was a problem hiding this comment.
Hi Christina, great job on implementing the requested changes - I can see a great improvement in this version!
I will approve your work for this week but I still noticed that when running the app locally, I can see a list of warnings by the compiler. Some of the warnings are mostly cosmetic (i.e. the img alt warnings) but others can lead to your app behaving unexpectedly in some scenarios. I would like you to go through the list and see if you can resolve them all so the app compiles without any warnings before the next homework submission ;)
| setError("Can not fetch data!"); | ||
| }); | ||
| setLoading(false); |
There was a problem hiding this comment.
Because the fetch function is asynchronous, you are ultimately setting the loading variable to false immediately, before any data has been fetched. Calling setLoading(false) inside the second then and catch blocks was correct.
To test if this works as desired, try opening the app in your browser while the throttling is enabled and check if the loading state is showing while the data is being fetched.
| const fetchProduct = async () => { | ||
| setLoading(true); | ||
| setError(null); | ||
| try { | ||
| const res = await fetch(`https://fakestoreapi.com/products/${id}`); | ||
| if (res.status !== 200) { | ||
| throw new Error("Something Went Wrong!"); | ||
| } | ||
| const data = await res.json(); | ||
| setProduct(data); | ||
| } catch (error) { | ||
| setError(error.message); | ||
| } | ||
| setLoading(false); | ||
| }; | ||
|
|
||
| useEffect(function () { | ||
| fetchProduct(); | ||
| }, []); |
There was a problem hiding this comment.
The fetchProduct function has a dependency (id) so calling it inside the useEffect hook without including the dependency in the dependency array will cause a warning and can lead to issues later when it will i.e. be possible to navigate directly from one product detail page to another.
To solve this, I recommend moving the fetchProduct function inside the useEffect hook and adding id variable in the dependency array.
There was a problem hiding this comment.
I'd recommend initializing the loading variable as true - similar to other components.
No description provided.