-
-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Html css popup #202
Html css popup #202
Conversation
Hey! Congratulations with your PR! 😎😎😎 Please, be sure you haven't followed common mistakes. Also, be aware, that if you would silently ignore this recommendation, a mentor may think that you are still working on fixes. And your PR will not be reviewed. 😒 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
Please, remove file submissions/anishchenko/html-css-popup/css/normalize.min.css, it is unnecessary for reiew.
See my comments below
input[type="checkbox"].visualy_hidden, | ||
input[type="radio"].visualy_hidden { | ||
position: absolute; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, remove empty lines in blocks, you don't need them
::-webkit-scrollbar { width: 0.3rem; height: 0.3rem;} | ||
::-webkit-scrollbar-button { background-color: #e1eaea; } | ||
::-webkit-scrollbar-track-piece { background-color: #fff} | ||
::-webkit-scrollbar-thumb {background-color: #666; border-radius: 0.3rem;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, keep one formatting style in whole file
display: flex; | ||
} | ||
|
||
/* scroll */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid unnecessary comments
|
||
/* media */ | ||
|
||
@media screen and (max-width: 36rem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you use relative units of measurement in media queries?
<li class="popup__item"> | ||
<a class="popup__link" href="#"> | ||
<figure class="popup__figure"> | ||
<img class="popup__img" src="img/icons/calendar.png" alt="calendar" width="40" height="30"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For icons it's better set an empty alt attribute, read more here
done! |
header { | ||
margin-top: 0.8rem; | ||
} | ||
|
||
main { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have classes header
and main
, why you don't use them in css?🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i hope that i fix this problem)) thanks for review
* add html-css-popup to folder anishchenko * fix bugs * delete normalize.css * added more content in main and footer sections * fix: use classes in css
html-css-popup
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.