MOYO header added task solution #113
Conversation
aRabiej
commented
Jun 30, 2023
aRabiej
commented
- DEMO LINK
- TEST REPORT LINK
| @@ -1 +1,75 @@ | |||
| /* Styles go here */ | |||
| body { | |||
There was a problem hiding this comment.
One of the task requirements is to reset browsers default styles, setting margin: 0; only on the body is not enough. You reset the styles either by setting default values on all elements
* {
margin: 0;
}or by creating a new file reset.css with for example Meyer Reset and importing it to the main one -> this way is better as in meyers reset you have complete control over what elements you modify as they are all listed instead of blindly setting values with *
src/index.html
Outdated
| </head> | ||
| <body> | ||
| <h1>Moyo header</h1> | ||
| <header class="navigationBar"> |
There was a problem hiding this comment.
Rather than using camel case for the classes you could use BEM methodology for naming, here it's optional however in later prs you will have to use it
| <header class="navigationBar"> | ||
| <a href="#home" class="nav_link"> | ||
| <img src="images/logo.png" alt="icon of the website"> | ||
| </a> |
There was a problem hiding this comment.
Here between the a and nav there should be an empty lines as they are siblings.
Generally there are a few rules regarding spacing your tags for the sake of codes redability:
- siblings should have an empty line between each other
<div>
<span></span>
<p></p>
</div>there are however a few exceptions from that rule, for example when you have a bunch of repeated tags like in your ul below:
<div>
<p></p>
<p></p>
<ul>
<li></li>
<li></li>
<li></li>
<li></li>
</ul>
</div>
src/index.html
Outdated
| <nav class="nav"> | ||
| <ul class="nav_list"> | ||
| <li class="nav_item"> | ||
| <a href="https://" class="is-active nav_link">Apple</a> |
There was a problem hiding this comment.
These links now lead to a non existent site empty site, it would be better for them to lead to ids based on their values, here it would be -> href="#apple"
src/style.css
Outdated
| } | ||
|
|
||
| .is-active { | ||
| color: #00acdc; |
There was a problem hiding this comment.
You are using this color 3 times, so it would be good to use here css variables to make color modification easier.
Create variable for the color :)
There was a problem hiding this comment.
Bump this comment you can declare a variable in .css as example: --blue: #1e90ff; and then you use it where do you need as an example color: var(--blue); or background-color: var(--blue);
src/style.css
Outdated
| padding: 0; | ||
| margin: 0; |
There was a problem hiding this comment.
You can remove this padding and margin from this selector as your reset.css will do the job of reseting padding and margins:
| padding: 0; | |
| margin: 0; |
src/style.css
Outdated
|
|
||
| .nav_item { | ||
| position: relative; | ||
| padding: 0; |
There was a problem hiding this comment.
Same here
| padding: 0; |
src/style.css
Outdated
| .icon { | ||
| width: 40px; | ||
| height: 40px; | ||
| top: 10px; | ||
| left: 50px; | ||
| position: absolute; | ||
| margin: 0; | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
This class is not used - remove outdated codebase
src/style.css
Outdated
| @@ -1 +1,62 @@ | |||
| /* Styles go here */ | |||
| body { | |||
| margin: 0; | |||
There was a problem hiding this comment.
You don't need this rule if you imported reset.css file :) it already has that
src/style.css
Outdated
| } | ||
|
|
||
| .is-active { | ||
| color: #00acdc; |
There was a problem hiding this comment.
Bump this comment you can declare a variable in .css as example: --blue: #1e90ff; and then you use it where do you need as an example color: var(--blue); or background-color: var(--blue);
