HTML CSS Module Project #660
Conversation
Merged 'level2' into 'master' and add new features without errors and bugs
✅ Deploy Preview for cyf-module-project-html-css ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
The page looks really nice. Well done.
Left some comments. Feel free to ignore them if not enough time. They are just recommendations.
If you will be answering to my code comments, pls try to answer to the particular comments. I mean, not by a single big answer in the end. It's hard to find in the big answer what part is related to what)
Also please write your own comments in the PR whenever you want me to pay attention to a particular place. Maybe you have difficulties in there or you just feel proud and want to share the solution
| margin-right: 30px; | ||
| } | ||
| .burger-button { | ||
| display: none; |
There was a problem hiding this comment.
Why is it hidden? Just work in progress? Or it's not needed for this design?
There was a problem hiding this comment.
Nice Question!
"The burger button is hidden because it's unnecessary when the page is displayed in the web view (for larger screens). I activate the burger button for mobile displays using media queries. This way, it's only visible and usable when the page is viewed on a mobile device."
| header { | ||
| height: 100px; | ||
| display: flex; | ||
| position: relative; |
There was a problem hiding this comment.
Did you already learn the differences between position static, relative, absolute, etc?
Why do you need relative in this case?
There was a problem hiding this comment.
"I used position: relative on the header because I want to reposition the navigation when applying media queries for mobile devices. On mobile, the navigation (header nav) is set to position: absolute, allowing me to control its position using the top, bottom, left, and right properties. By setting the parent (header) to position: relative, I ensure that the absolutely positioned navigation is placed relative to the header, making it easier to position the nav exactly where needed for mobile layouts."
css/store.css
Outdated
| display: flex; | ||
| position: relative; | ||
| } | ||
| header :nth-child(2) { |
There was a problem hiding this comment.
A similar feedback as in the past. nth-child in css is unreadable. Also it's problematic to use in more real life scenarios when content is added dynamically to the page.
Can you think of a way to avoid using it in here?
There was a problem hiding this comment.
"It's a bad habit of mine to use nth-child without fully considering the potential issues, but I'll make sure to avoid it in the future and look for more reliable solutions."
css/store.css
Outdated
| border-radius: 10px; | ||
| box-sizing: border-box; | ||
| } | ||
| .address11 { |
There was a problem hiding this comment.
address11 and address12 are exactly the same.
We can use just address.
There was a problem hiding this comment.
Again try to create reusable classes. What I wrote in another comment.
Class is "one of". For example "One of address fields"
Whenever there is a class address1 in your code, it's a sign that we need two classes in there:
- Common for all address fields;
- A modifier for a particular address field.
There was a problem hiding this comment.
Continuing with the address example.
Let's say you have 2 address fields. The first is required and has to be filled, the second is optional.
Instead of creating classes "address1" and "address2", a better way is to create classes "address" and "required".
This way if you later need a third address field, you don't need to edit your css. You will just reuse the "address" class.
Also maybe you will have other required fields having some similarities. Then the "required" class can be reused as well.
There was a problem hiding this comment.
A bit more on the "required" class for address.
You could add "*" in css. It would be something like this:
.required::after {
content: '*';
}
Haven't tested this code. Please tell if you will try and it doesn't work. Can look into it.
There was a problem hiding this comment.
"::after" above is a so called "pseudoelement" You can read more in here for example https://developer.mozilla.org/en-US/docs/Web/CSS/::after
There was a problem hiding this comment.
"Yes, it's a good approach to use just one class instead of multiple. I completely agree with you, and I'll work on resolving it. Thank you so much for your support!"
There was a problem hiding this comment.
A bit more on the "required" class for address. You could add "*" in css. It would be something like this:
.required::after { content: '*'; }Haven't tested this code. Please tell if you will try and it doesn't work. Can look into it.
"I used the .quote class to automatically add quotation marks before and after the content, and it works well. I’ve tested it, and everything seems fine."
store.html
Outdated
| <legend>Select a Color:</legend> | ||
| <div class="radio-group"> | ||
| <div class="radio"> | ||
| <input type="radio" id="orange" name="orange1" value="orange"> |
There was a problem hiding this comment.
Only one color can be selected, right?
You need to use the same name for all the color inputs. "color" for example
There was a problem hiding this comment.
"Right, I will correct it "
…e class inside header nav
|
It all looks good to me Syed. Well done man. |
Your Details
Homework Details