ILIA_BUBNOV-w2-React #12
Conversation
…t Error and clean up
There was a problem hiding this comment.
Hi Ilia,
Great work on this assignment. Your code is well structured and organised.
Please make sure you read the README before you submit your code to double-check you've handled all requirements. You'll have to do the same thing as a professional developer before you hand your code over to testers.
| @@ -0,0 +1,30 @@ | |||
| .productPage { | |||
There was a problem hiding this comment.
Have you viewed this page in mobile view? The layout is not very user-friendly. 🟠
| @@ -0,0 +1,26 @@ | |||
|
|
|||
There was a problem hiding this comment.
Good idea to create separate util functions!
| function Products({ selectedCategory }) { | ||
| const [products, setProducts] = useState([]); | ||
| const [isLoading, setLoading] = useState(true); | ||
| const [currentError, setError] = useState(null); |
There was a problem hiding this comment.
Naming convention for react state is variable, setVariable. 🔵
| padding: 0; | ||
| } | ||
|
|
||
| h1 { |
There was a problem hiding this comment.
Please fix this issue from last week's assignment: #1 (comment)
There was a problem hiding this comment.
if you mean letting rem be equal to 16px, I strongly disagree with that, and I will argue that the link you provided does not clarify or prove anything, as it is just a discussion on Reddit. I would like to keep it that way and if in future I come to work in a project that requires me to have rem be set to 16px I will, but as this project is (kinda) my own, I don't see a reason to change it.
There was a problem hiding this comment.
if it is about something else, could you please provide more feedback
There was a problem hiding this comment.
I was referring to the global settings for html elements, e.g. all your headings tags. This is bad practice as it will make styling harder in the future. Maintainable apps apply styles where they're needed and not globally.
As for the root font size: as I said, you can keep it at 62.5% if you like. I can send you plenty of articles if you are curious about the debate between 10px root font size and 16px root font size.
| border-radius: 5px; | ||
| border: 1px solid black; | ||
| } | ||
| .categoryLi:hover { |
There was a problem hiding this comment.
You have three different background states for your button interactions and it's a little confusing for the user.
You apply a dark grey bg on hover, then revert to default colours during the click, then apply the same dark grey bg when the category has been selected. Is this what you intended?
Convention is the following:
- Default colours
- Different colour on hover
- Another different colour on click (optional)
- Another different colour when selected.
🟠
| try { | ||
| setLoading(true); | ||
| const products = await fetchProducts(); | ||
| setProduct(products.find(product => product.id === Number(id))); |
There was a problem hiding this comment.
You are missing a requirement from the README:
- Your app needs to go to a detail page
/product/:idwhenever you click on the product card in the list. This should get the details from the endpoint: `https://fakestoreapi.com/products/
🔴
There was a problem hiding this comment.
I don't understand what you mean, my fetcher function does precisely that, every time the product page opens it fetches all the products and filters out the product that we need. If I missed something, could you please clarify what it is I am doing wrong?
There was a problem hiding this comment.
Check our README and the documentation of the API - there is a way to get exactly the product you need without doing filtering on the frontend.
|
Thank you for your feedback and your patience. React module is kinda frustrating, and we have a lot of reading materials that are not very reader-friendly. Just to clarify, I may have some strong positions. I hope my comments didn't bother you, as I only wish to get to the truth, and sometimes it requires a little debating. I hope you understand. |
|
@MBreathe not at all - I enjoy a robust discussion about coding principles! I hope you also understand my role is to give you the very best information I can in order to set you up for success at your career. Can you let me know which reading materials were not easy to work with, or which concepts are still confusing? |
|
I believe that the only thing that I struggle with is the concepts. We had a lot of new information in this module, and most of it was just reading documentation. I think that just 4 weeks are not enough for React, it's just way more complex than what I imagined, and even with a little prior experience working in React, I struggled in this module (this was the only one I had real struggles with). But also, I have to be honest, sometimes I miss some crucial things that are required, because I am rushing all the time (a thing that I am working on). Also, with CSS, especially, I am writing it based on my feeling of what's right because we haven't had any materials covering it. I know, it is not that deep, but still would've been nice to know the standards from the course and not to have to guess what is right on the internet. |
https://ilya-ecommerce.netlify.app/