popup #7
popup #7
Conversation
There was a problem hiding this comment.
@maximmorenko please undo changes to .github/pull_request_template.md. You were supposed to change the Pull Request initial message body.
Извините, но я не понимаю что я не так сделал. В пункте 5 написано, что нужно изменить в этом шаблоне. Возможно я неправильно понял, если не в этом файле, то где мне оставить ссылки на проект и на репо?
| <header> | ||
| <nav class="nav"> | ||
| <div class="nav-item logo"> | ||
| <a href="#"><img src="./assets/icons/001_logo.png" alt="logo"></a> |
There was a problem hiding this comment.
Logically this logo looks like not a part of navigation what you think about that?
There was a problem hiding this comment.
I think that the logo can be a link to the main page, which means it is a navigation element) but if it's a problem, then I can remove it
There was a problem hiding this comment.
In 99.999% of cases, logo must be a link and leads to the home page. Logo is a reusable component, better keep it outside navigation. Also if we will have another block of links (login/signup etc.) should we add him into this navigation, or keep hin as an independent component?
There was a problem hiding this comment.
компонент с функцией навигации изъять из списка навигации, ок) вам виднее)
|
|
||
| <div class="wrapper-popup wrapper-popup-hidden"> | ||
| <ul class="main-content"> | ||
| <li class="popup-item"><a href="#" ><img src="./assets/icons/sport/01.png" alt="a"></a></li> |
There was a problem hiding this comment.
Read more about alt attribute, sorry for russians but it's a good article
There was a problem hiding this comment.
да, я прекрасно понимаю, что альт должен описывать картинку) но ведь там два десятка иконок, мне показалось что на учебном проекте не принципиально давать каждой своё описание. или нет)? sorry for russians
There was a problem hiding this comment.
According to an article, you must fill alt attribute for content images. the icon is a decorative image(not content) and for him, alt must be empty. Read more carefully.
| <main></main> | ||
| <footer></footer> | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
Add empty lines at the end of files. Reason
There was a problem hiding this comment.
причины нет) предполагалось что что-то будет в будущем
There was a problem hiding this comment.
I mean word Reason is a link to page with explanation why you should add an empty line at the end of each file.)
There was a problem hiding this comment.
не совсем понял) должна появиться пустая 74я строка?
There was a problem hiding this comment.
Yes, add the line.
An empty line at the end of the file avoids the wrong concatenation of files.
There was a problem hiding this comment.
пустую строку добавил еще вчера, но она не отображается в коммите.
что делать? где я не прав?)
There was a problem hiding this comment.
By default, Github hides the last empty line.
If you see this icon at the end of your file that means you forget to add an empty line, and you should add it.
| align-items: center; | ||
| background-color: var(--colors-bg); | ||
| } | ||
|
|
There was a problem hiding this comment.
Try to keep your code style cleaner: space between code blocks, no unused lines inside code block(in CSS)
добавил альт, навел порядок
исправил скрытие чекбокса, навел порядок
| @@ -151,6 +149,7 @@ ul { | |||
| cursor: pointer; | |||
| /* прячем чекбокс */ | |||
| -webkit-appearance: none; | |||
| -moz-appearance: none; | |||
There was a problem hiding this comment.
When you use CSS properties with vendor prefixes is considered good practice to write properties with vendor prefixes and at the last line property without. Example:
.thing {
-webkit-appearance: value;
-moz-appearance: value;
appearance: value;
}
| </div> | ||
| </li> | ||
| <li class="nav-item"> | ||
| <a href="#"><img src="./assets/icons/03_baseline_email.png" alt="email"></a> |
There was a problem hiding this comment.
Why do you leave the filled alt attribute for these icons but remove it for icons inside popup?
There was a problem hiding this comment.
я их прозевал) извините
|
If you want to discuss something in voice we can schedule a call in discord and discuss what you want. |
теперь, кажется все альты пустые) и строка 68 тоже есть
|
Well done! |
* make 'side-menu' task * refactor: fix hover effects * refactor: get rid of comments, make js file works in strickt mode * refactor: fix typo in 'use strict', change searching elements by id to searching by class name, make variables constanst where it is appropriate, get rid of css ctyles in js file * refactor: give element more meaningfull names, created new content elements insted of updating old ones * refactor: make content be generated, not updated; don't generate an empy link and img after reset content. * refactor: use array.find() instead of array.findIndex() * refactor: make small css fixes to make better element's positioning and adaptivity
task-popup
Demo |
Code base