add solution #4909
add solution #4909
Conversation
There was a problem hiding this comment.
Good job 👍 But we need to do few fixes
Please, read carefully the checklist to the task checklist to the task
src/style.css
Outdated
| margin: 10px auto 10px 50px; | ||
| } | ||
|
|
||
| nav { |
There was a problem hiding this comment.
Please, check the checklist to the task . Let`s use classes instead of tags for selectors . Fix it through all your file
src/index.html
Outdated
| <ul class="noPadding"> | ||
| <a | ||
| class="link is-active" | ||
| href="APPLE" | ||
| > | ||
| APPLE | ||
| </a> | ||
| </ul> | ||
|
|
||
| <ul> | ||
| <a | ||
| class="link" | ||
| href="SAMSUNG" | ||
| > | ||
| SAMSUNG | ||
| </a> | ||
| </ul> |
There was a problem hiding this comment.
Let's create one list(ul) and use li tags for it`s children, that is how lists work
| <ul class="noPadding"> | |
| <a | |
| class="link is-active" | |
| href="APPLE" | |
| > | |
| APPLE | |
| </a> | |
| </ul> | |
| <ul> | |
| <a | |
| class="link" | |
| href="SAMSUNG" | |
| > | |
| SAMSUNG | |
| </a> | |
| </ul> | |
| <ul class="noPadding"> | |
| <li class="list__item"> | |
| <a class="link is-active" href="APPLE"> | |
| APPLE | |
| </a> | |
| </li> | |
| <li class="list__item"> | |
| <a class="link" href="SAMSUNG"> | |
| SAMSUNG | |
| </a> | |
| </li> | |
| </ul> | |
src/style.css
Outdated
| width: 100%; | ||
| box-sizing: border-box; | ||
| background-color: rgb(255, 255, 255); | ||
| max-height: 60px; |
There was a problem hiding this comment.
No need for this line, it is said in the task to set height of 60px only to links
src/style.css
Outdated
| display: flex; | ||
| text-align: center; | ||
| margin: 0 50px 0 auto; | ||
| height: 60px; |
There was a problem hiding this comment.
Same here, remove it from here and add to the link
src/style.css
Outdated
| border: 1px solid #00acdc; | ||
| display: flex; | ||
| position: absolute; | ||
| margin-top: 19px; |
There was a problem hiding this comment.
and if you move the height property to the link instead of nav, you wont need to set margin here
| .link { | ||
| text-decoration: none; | ||
| color: black; | ||
| padding: 23px 0; |
There was a problem hiding this comment.
Use flexbox, not padding to center the text
There was a problem hiding this comment.
Better ⭐ but still few fixes needed
It is said in the task to set the height to links not the nav itself, please fix it
Also run npm run deploy, because the link shows your old code
Sorry, just thought I did not pressed re-request bottom) Did not expect you check it so soon) |
DEMO LINK
TEST REPORT LINK
Yellow stars are added with container modifier + pseudo-selector (NO extra classes)
Each BEM block has its own separate file
All Typical Mistakes from BEM lesson theory are checked.
Code follows all the Code Style Rules ❗️