ILIA_BUBNOV-w1-React #1
Conversation
.idea/vcs.xml
Outdated
| @@ -0,0 +1,6 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
what are the files in this folder for?
There was a problem hiding this comment.
This is the folder that helps set up the workspace in WebStorm; I should not have pushed that. Thank you for pointing it out
| font-size: 62.5%; | ||
| font-family: sans-serif; | ||
| } | ||
| * { |
There was a problem hiding this comment.
Be careful about applying styles to all elements or all elements of one type across your application. There may come a time when you want to use different styles, and that will be very difficult because of the styles you've set here. 🔵
There was a problem hiding this comment.
If you're talking about font-family, then yes, I agree, but when it comes to font-size, I only set it to 10px so that I can use rem throughout the whole project effectively. At least that what I've learned from my senior mentor
There was a problem hiding this comment.
Interesting. I'll bring this up with the other mentors, because it's something that is not always advised. I believe that we should stick to 16px. Here's a good series of comments as to why that's a good idea (in my opinion): https://www.reddit.com/r/webdev/comments/fhfe0n/when_using_rem_is_it_better_to_have_a_16px_base/
I will let you do what you think is best with the font size. As for the other css properties (not just on * but also on h1, li, span) you should consider the maintainability of your project going forwards - this is what employers will consider as well.
| display: flex; | ||
| flex-wrap: wrap; | ||
| justify-content: center; | ||
| cursor: pointer; |
There was a problem hiding this comment.
We usually only apply cursor: pointer and hover effects when the element is clickable. In this case, your cards aren't clickable, so you should remove them (for now!) 🟠
There was a problem hiding this comment.
Yea I was actually thinking ahead here
| border-radius: 5px; | ||
| box-shadow: 0 0 10px rgba(55, 55, 55, 0.5); | ||
| } | ||
| .products ul li img { |
There was a problem hiding this comment.
Instead of targeting the html elements within a class, you should simply set a class on the element you want to target. This will do a couple of things: 1. It will make it clearer which targets you're applying styles to, 2. It will increase the specificity of your styles. 🟠
| @@ -0,0 +1,12 @@ | |||
|
|
|||
There was a problem hiding this comment.
Make sure you enable "Format on save" in your IDE (VSCode, probably?) so that your eslint style rules get applied 🔵
| @@ -0,0 +1,27 @@ | |||
| .navbar { | |||
| .navbar ul { | ||
| list-style: none; | ||
| } | ||
| .navbar ul li { |
There was a problem hiding this comment.
I have the same suggestion in this file as I have in products.module.css 🟠
There was a problem hiding this comment.
I was actually thinking of getting rid of all .module.css files and moving all the components to one directory, because the project isn't big enough to set up individual styling for components
| import categories from "../fake-data/all-categories.js" | ||
| import style from "./navbar.module.css" | ||
|
|
||
| function categoryClickHandler(e) { |
There was a problem hiding this comment.
By accessing innerText you are going around React's virtual DOM. React has knowledge of everything in virtual DOM, but not the actual DOM on the page. So if you do things like document.querySelector or textContent or innerText, you are making React interact directly with the DOM. This should be avoided where possible. https://talent500.com/blog/virtual-dom-react-explained/
Try moving the click handler inside the component. You will have access to all the information you need about categories - including which one has been clicked. 🔴
There was a problem hiding this comment.
Thank you for your comments I will get to implementing them later today
Link to deployement: https://e-commerce-ilya.netlify.app/